BIND 10 #308: review: simple benchmark for b10-auth query handling

[BIND 10 Development]

#308: review: simple benchmark for b10-auth query handling
------------------------------+---------------------------------------------
      Reporter:  jinmei       |        Owner:  jinmei              
          Type:  enhancement  |       Status:  reviewing           
      Priority:  minor        |    Milestone:  y2 6 month milestone
     Component:  b10-auth     |   Resolution:                      
      Keywords:               |    Sensitive:  0                   
Estimatedhours:  0.0          |        Hours:  0                   
      Billable:  1            |   Totalhours:  4.5                 
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 Replying to [comment:6 vorner]:
 > I have few questions there (but they might be due to that I'm new to the
 code):
 >
 > * query_bench.cc:60: Is the AuthServerPtr( ) needed? Doesn't naked
 pointer (* AuthServer) convert to it automatically? Or is it to make the
 code more explicit about it?
 >
 No, it was an error.  Thanks for catching it.  Fixed in r2959.

 > * query_bench.cc:79: The query variable is used only inside the for
 cycle. Is there a reason for it to be defined outside it?
 >
 Nothing special, but I wanted to keep the "for" statement in a single line
 (not exceeding 80 characters).  I don't mind changing the code but in this
 specific case I don't see much difference between these two, so unless I
 hear otherwise I'll keep the current code.

 > * auth_srv.h, the documentation for getCacheSlots and setCacheSlots says
 it throws no exceptions. Should the code say the same by void
 setCacheStlots(const size_t slots) throw();? Or it is not there due to
 speed reasons? And, btw, why is it const size_t, when size_t is kind of
 integer?
 >
 Personally, I don't use exception specifications.  See Item 14 of "More
 Effective C++" and/or Rule 75 of "C++ Coding Standards" for my reasons.
 We don't have a consensus about whether to use/rely on exception
 specifications: Jelte seems to prefer using them, for example.  We have
 discussed this before but didn't reach a consensus.

 It's not ideal, but right now coding policy is inconsistent on this point.
 What I do is: don't use exception specifications for code I write; don't
 touch existing exception specifications for code written by others (unless
 the specification is incorrect).

 > * What these two do when cache is not in use? Is it an error or nop?
 >
 Good question.  Documented in the code.  r2960.  I think the question
 should actually go to the hot spot cache code though.

 > * Benchmarks do not have tests, right? Because they are kind of test
 themself?
 >
 Another good point.  Right, it currently doesn't have tests (and as you
 can see it's at least not TDD:-).  I'm not sure if we require tests for
 benchmarks.  I personally think we *can* skip tests for benchmarks
 because, as you said, benchmarks are a kind of test themselves.  On the
 other hand, if the benchmark code does something tricky inside it, maybe
 we should require explicit tests to confirm the test code does the right
 thing.  Personally I'd suggest we consider it case by case basis.

 > * Is it run anywhere? I did not find any data for it or that kind of
 stuff, nor any code that calls it ‒ so it is for manual running?
 >
 Right now, it's only for manual running.  You need to prepare the input
 data and the data source file.  Eventually we might want to make it self-
 runnable for automatic tests, but I think manual tests are okay for now.

-- 
Ticket URL: <http://bind10.isc.org/ticket/308#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development