BIND 10 #2042: Prototype backend performance microbenchmark: MySQL

[BIND 10 Development]

#2042: Prototype backend performance microbenchmark: MySQL
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  marcin
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:         |  DHCP-20120903
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:12 marcin]:
 > I have reviewed commit 272d9f6f5bb929d81f1236ad953378d145225ade.
 >
 > '''General'''
 > Minor: In the code you're using mixture of std::cout and printf. Im my
 opinion it would be "cleaner" to use one of them all the time. In any
 case, this is not very important.
 All converted to cout.

 > '''tests/tools/dhcp-ubench/dhcp-perf-guide.html'''
 > The information in the document is very useful (especially the results
 presented on the graph).
 > One concern I usually have with benchmarks is how consistent the results
 are. In other words, if I run this test a couple of times in turn, would
 it give the same result?
 Good point. There's planned re-run with compiled statements implemented. I
 will rerun the tests 10 times and will provide average.

 > '''tests/tools/dhcp-ubench/benchmark.h'''
 > Suggest that void print_clock(...) is ranamed to printClock(...) so as
 it is consistent with naming convention used for other functions. The same
 applies to struct timespec get_time().
 Done.

 > In doxygen comments to several functions there is a reference to Num_
 member that does not exist. It should be replaced with num_ which is
 declared as uint32_t num_ in protected scope.
 Done.

 > '''tests/tools/dhcp-ubench/benchmark.cc'''
 > Although it is not a big deal when calling it once, but the following
 assignment hitratio_ = 0.9f in constructor's body could be moved to
 constructor's initialization list.
 Done.

 > parseCmdline(): Error string ''"Failed to iterations (-n option)."''
 does not make sense to me.
 >
 > parseCmdLine(): The lexical_cast<int> prevents specifying non-numeric
 arguments but it will not prevent negative arguments like -n-20. Suggest
 additional check that num_ > 0.
 Fixed.

 > usage(): Parameter -c is not documented.
 It is now documented.

 > get_time(): clock_gettime may return negative value which indicates
 error. This should be checked prior to returning ts. If it occurs it is
 fatal error.
 >
 > get_time(): This is a side note. I have been using
 boost::posix_time::ptime class which I found very useful and more portable
 than clock_gettime. With the latter you have to take care of ifdefs for
 particular OSes.
 Good information, but it does not offer enough precision. ptime is
 implemented on Linux using GetTimeOfDay, which offers microsecond
 resolution, while clock_gettime offers nanosecond resolution. Please note
 that in the most efficient cases we are able to do well over 1 million
 searches per second, so each individual operation may take less than a
 microsecond. It is useful to have clock resolution greater than the event
 we are trying to measure.

 (Of course, no sane person would measure just a single event, but it is
 good to keep the measurement error as low as possible).

 > '''tests/tools/dhcp-ubench/memfile_ubench.cc'''
 > The class members Filename_, Sync_ and File_ should have names starting
 with lowercase letter.
 >
 > createLease4Test(): Not very important here but it is recommended to use
 ++i in for() loops rather than i++. The same thing can be found in
 multiple other places too.
 That is true only if increased entity is an object, not a primitive type.
 For objects, a temporary object is created.

 > createLease4Test(): For people who are less familiar with the code it
 would be good to have comments what the magic numbers: 19, 65, 33 are. The
 other solution would be to define constants with meaningful names.
 Replaced 19 with "hwaddr_len - 1", 65 with 'A' and commented on 33.

 > searchLease4Test(): The variable hitRatio should be renamed to
 hit_ratio. Otherwise it suggests that hitRatio is a function.
 It now uses Benchmark::hitratio_, just as two other benchmarks do.
 >
 > '''tests/tools/dhcp-ubench/mysql_ubench.h'''
 > MySQL_uBenchmark constructor: descirption of parameter hostname says
 "Name of the hostname to connect to". Suggest that this is changed to
 "Name of the host to connect to".
 Done.

 > '''tests/tools/dhcp-ubench/mysql_ubench.cc'''
 > createLease4Test(): Comment at pool_id declaration does not make sense
 to me. Variable is initialized to 1000 while comments says 'Let's use
 pools 0-99'
 Comment corrected.

 > createLease4Test(): Again, it would be good to have some more comments
 describing magic numbers: 19,65 etc.
 Done.

 > '''tests/tools/dhcp-ubench/sqlite_ubench.cc'''
 > disconnect(): Function closes connection if db_ object is does nothing
 if db_ object does not exist. It could be better maybe to throw exception
 (say InvalidOperation) if db_ does not exist to inform caller that nothing
 has been done?> The mysql_ubench::disconnect() behaves that way.
 An exception is now thrown. We can't use typical exception types used in
 BIND10, as this code is an independent tool (no dependencies on any other
 BIND10 code).

 > search_callback(): Shouldn't be the code under #ifdef 0 executed in
 verbose mode and not executed in non-verbose?
 No. Verbose only tracks progress by printing dots. Printing lease content
 when search is executed 1000s of times would be overkill. The code has
 been modified to actually consume contents of found lease.

 Thanks for thorough review.

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