Opened 9 years ago

Closed 9 years ago

#241 closed enhancement (fixed)

review: benchmark framework

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: 06. 4th Incremental Release
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

Now that we've started some performance work, I propose we merge my micro benchmark framework from the experiments/jinmei-onmemdb branch to trunk.

It would be needless to say that measurement is crucial for any performance related work, but another important point in having a handy benchmark tool is it will help us avoid jumping into the root of all evil (aka premature optimization).

I'm going to make a branch for that. Hopefully it's a trivial work and can be quickly reviewed and merged before the next release.

Subtickets

Change History (6)

comment:1 Changed 9 years ago by shane

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

Since I don't see any more comments, I will assume this did not get done, and am moving it to the next release.

comment:2 Changed 9 years ago by jinmei

  • billable set to 1
  • Internal? unset

branches/trac241 is ready for review.

Diff can be retrieved by:

svn diff -r 2137 svn+ssh://bind10.isc.org/svn/bind10/branches/trac241

This is independent from any other part of the BIND 10 source tree,
and, essentially, the only things to be reviewed are files under
src/bench/.

Assuming (and hopefully) this framework is useful for other
developers, I wrote detailed documentation including how to use it.
An html version of the doxygen doc is available at:
http://bind10.isc.org/~jinmei/bind10/trac241/cpp/namespaceisc_1_1bench.html

Proposed changelog entry is:

  76.?	[func]		jinmei
	Added a C++ framework for micro benchmark tests.  A supplemental
	library functions to build query data for the tests were also
	provided. (Trac #241, svn rTBD)

comment:3 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing
  • Summary changed from benchmark framework to review: benchmark framework

comment:4 follow-up: Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jinmei

Had a discussion on jabber about the changes; summary is that I see no reason not to merge it as it is, with the reservations i had being added to the documentation as future work.

--

(07:06:57 PM) jelte: i was wondering, should we have separate directories like we do for normal unittests?
(07:08:11 PM) jelte: the thing being that benchmark_util contains mostly libdns++-specific stuff :)
(07:09:23 PM) jelte: also i was thinking if we could make it more like gtest (in that we wouldn't need to define main(), and for trivial cases not even subclass something, but have macros), but that could perhaps be added later, in general i think the code looks ok afaict
(07:09:25 PM) jinmei: as for separate directories I don't have a strong opinion, but the initial idea was to make separate benchmark directories for different modules
(07:09:37 PM) jinmei: e.g. src/lib/dns/benchmarks
(07:09:37 PM) jelte: right, i thought so
(07:09:56 PM) jinmei: I'm not sure if the second point is related to the directory issue
(07:09:59 PM) jelte: making the benchmark dir not much more than that header file :)
(07:10:02 PM) jelte: no it isn't :)
(07:10:25 PM) jinmei: I
(07:10:31 PM) jinmei: oops mistyped
(07:10:37 PM) jelte: but it occurred to me while i was reading through the example again :)
(07:11:35 PM) jinmei: as for the second point, I know that.
(07:12:32 PM) jinmei: maybe we'll have other benchmark utils that are not so specific to DNS here.
(07:13:00 PM) jinmei: e.g. we may want to do some network I/O specific benchmarks and we want to have utilities for that
(07:13:20 PM) jinmei: but honestly I don't know how it will evolve
(07:14:00 PM) jelte: well if we keep the framework separate, and decide on lib/dns/benchmarks/ than what's currently in benchmark_util.* could go there i think
(07:14:27 PM) jelte: yeah that was my other thing, it looks ok to me right now, but once we start writing other benchmarks we're bound to run into things :)
(07:15:18 PM) jinmei: ah
(07:15:46 PM) jinmei: first off, the current benchmark_util is actually not intended to be used for libdns++.
(07:16:05 PM) jinmei: the intended primary intended user is the auth server
(07:16:26 PM) jinmei: with clarifying this,
(07:16:45 PM) jinmei: the question is whether this should better be placed in src/bin/auth/benchmarks for example.
(07:16:52 PM) jelte: right
(07:17:15 PM) jinmei: again, I don't have a strong opinion,
(07:17:56 PM) jinmei: but it's quite likely we'd want to have something like this for the caching (recursive) server.
(07:18:09 PM) jelte: noting that some of our unit tests are still not living in their own directory :)
(07:18:31 PM) jinmei: yeah, I've been aware of that
(07:19:15 PM) jinmei: as for making it gtest-like, yes, that makes sense
(07:19:59 PM) jinmei: and I sort of tried to achieve that type of goal in the initial design/implementation,
(07:20:08 PM) jinmei: and I also admit this could be improved
(07:21:32 PM) jelte: it does not have to stop us from including this as it is
(07:22:23 PM) jinmei: so, for now, I'd add some description to the doxygen comments:
(07:22:54 PM) jelte: oh and a more code-specific issue; benchmark.h can use some empty lines :)
(07:23:03 PM) jinmei: - the design of benchmark.h may be changed so that it will be set up more easily.
(07:23:42 PM) jelte: every function right now is immediately followed by the doxygen for the next one, which makes reading through it a bit harder than it needs to be
(07:23:43 PM) jinmei: - benchmark_util may be too specific to be placed here, and we'll see if it should stay here or move to more appropriate benchmark cases.
(07:23:54 PM) jinmei: okay
(07:24:01 PM) jinmei: > empty lines
(07:24:51 PM) jinmei: maybe we should add this to coding guideline?
(07:25:00 PM) jinmei: or is it too minor?
(07:26:25 PM) jelte: perhaps we should, i have also been reprimanded by michael for not using enough vertical space at one point :)
(07:27:43 PM) jinmei: blank lines added (r2660)
(07:29:17 PM) jelte: much better :)
(07:39:13 PM) fujiwara entered the room.
(07:40:51 PM) jreed: "style fix: added blank lines between blocks of (function + its description)" sounds good for style guide too.

(07:41:39 PM) jreed: not sure if it is important for following function, but for a gap before previous lines.
(07:45:17 PM) jinmei: added a note about future possible extension to the benchmark (i.e. making it gtest-like).
(07:45:19 PM) jinmei: r2661
(07:48:21 PM) jelte: looks good

comment:5 in reply to: ↑ 4 Changed 9 years ago by jinmei

Replying to jelte:

Had a discussion on jabber about the changes; summary is that I see no reason not to merge it as it is, with the reservations i had being added to the documentation as future work.

Thanks for the review.

I believe the points are addressed in r2660, r2661, and r2662.

Regarding where to place actual benchmarks, I think we can show it by convention. I'll soon add some specific benchmarks under src/bin/auth/benchmarks and src/lib/dns/benchmarks.

comment:6 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

Got an okay on jabber. Merged to trunk, closing.

Note: See TracTickets for help on using tickets.