BIND 10 #2042: Prototype backend performance microbenchmark: MySQL

[BIND 10 Development]

#2042: Prototype backend performance microbenchmark: MySQL
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:7 stephen]:
 > Reviewed commit 6b80e1e57145e8b0796d872f51ca39a0f4bbc17a
 >
 > Note: a number of small changes (mainly to comments and word spacing)
 have been made and the results pushed to the repository.
 Thank you.


 > '''General'''[[BR]]
 > If these benchmarks are going to form part of the BIND-10 distribution,
 then the source files do need comments and the methods need unit tests.
 (The absence of unit tests was agreed on the assumption that they were not
 going to be distributed.)
 I take it as a suggestion to write missing comments. The code is now
 properly documented.

 > It is OK that they do not form part of the overall BIND 10 build - they
 are completely separate and !MySql is not likely to be installed on all
 systems.
 Ok.

 > The most important change needed to get realistic results is to try
 prepared statements in the database measurements.
 Still TBD.

 > '''tests/tools/dhcp-ubench/Makefile'''[[BR]]
 > When looking at it, my editor complained about the lack of a newline at
 the end of the file, so I added one.  I've also corrected a couple of
 minor errors concerning which flags are on which command line.
 Thank you.

 > '''tests/tools/dhcp-ubench/README'''[[BR]]
 > OK, but I think there could perhaps be a fuller explanation if this will
 be part of the distributed code.
 The idea here is to have a single point where detailed documentation is
 maintained (the performance guide) with other types of documentation (like
 README) kept to absolute minimum. All the details are in the performance
 guide. It will be much easier to maintain in the long run.

 > '''tests/tools/dhcp-ubench/benchmark.cc'''[[BR]]
 > When parsing an integer, suggest that boost::lexical_cast is used.  The
 problem with strtol() is that it ignores trailing invalid characters in a
 number (although they can be detected by checking the value returned in
 the endptr variable).
 Done.

 >
 > When parsing command switches 's' and 'v', the "if" block could be
 replaced by:
 > {{{
 > <variable> = !strcasecmp(optarg, "yes") || !strcmp(optarg, "1");
 > }}}
 That is shorter, but less readable IMHO. Done as suggested.

 > There is no need for the "case ':'" immediately before "default" - the
 latter covers it.
 Removed.

 > '''tests/tools/dhcp-ubench/benchmark.h'''[[BR]]
 > '''tests/tools/dhcp-ubench/mysql_ubench.h'''[[BR]]
 > '''tests/tools/dhcp-ubench/sqlite_ubench.h'''[[BR]]
 > '''tests/tools/dhcp-ubench/memfile_ubench.h'''[[BR]]
 > Member variable names should start with a lower-case letter.
 Converted all member variables to lower-case with trailing underscore.

 > Members of Lease4 structure should include inline comments.
 Lease4 structure is now properly documented.

 > getLease(): "if" block should be enclosed in braces.
 Fixed.

 > '''tests/tools/dhcp-ubench/mysql_ubench.cc'''[[BR]]
 > The variables initialized in createLease4Test() look a lot like those in
 the memory benchmark: they could perhaps be member variables in the base
 class and initialized in the constructor.
 Using exactly the same values in all back-ends serve no purpose, except
 aesthetic. It does not impact performance in any way. Refactoring the code
 to use exactly the same values in every benchmark will not be productive.
 If you think otherwise, please create separate ticket for that.


 > hitRatio is 0.5 for the memory benchmark and 0.9 for the !MySql one.  To
 eliminate some uncertainty in the comparison, they should be the same for
 each.  (In fact, the overall logic of the benchmark could be in the base
 class, with the access to the underlying database abstracted into methods
 in the database specialization classes).
 hitratio is now a member variable of the base class. Regarding moving
 overall logic of the base class - it would only complicate the design.
 Once the compiled statement support is implemented, where whould you
 suggest to keep compiled statement? It would have to be shared between
 calls. That is obviously a local information, so it should not be promoted
 to class variable.

 Again, it is a trade off between perfect design and time constraints.
 While I agree in principle that refactoring the code until it is a model
 C++ implementation would be nice, but I'm sure our customers perfer
 working DHCP server by end of the year.

 If you still think we have enough time for this, please create a ticket.

 > '''tests/tools/dhcp-ubench/sqlite_ubench.cc'''[[BR]]
 > Also applicable here are the points made in the mySql about
 createLease4() data initializations and the hitRatio value.
 hitratio_ moved to base class.

 That is a response with all comments, except compiled statements. I will
 address that in the next comment.

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