Opened 9 years ago

Closed 9 years ago

#308 closed enhancement (fixed)

review: simple benchmark for b10-auth query handling

Reported by: jinmei Owned by: jinmei
Priority: low Milestone: y2 6 month milestone
Component: b10-auth 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: 0
Total Hours: 5.2 Internal?: no

Description


Subtickets

Change History (15)

comment:1 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 2.0
  • Total Hours changed from 0.0 to 2.0

comment:2 follow-up: Changed 9 years ago by jinmei

Please review branches/trac308. The branch point is r2717.

I've also a bit tweaked the AuthSrv class so that I can specify the size of hot spot cache.

This is a proposed ChangeLog entry:

  85.	[func]		jinmei
	Added a micro benchmark test for query processing of b10-auth.
	(Trac #308, svn rTBD)

(The rest of this comment is not directly related to the patch itself, and can be ignored for the purpose of code review)

Just for information, here's a sample result using (an unsigned version of) root zone with a real query sample.

% ./query_bench -n 2 root.sqlite3 query.txt
Parameters:
  Iterations: 2
  Data Source: root.sqlite3
  Query data: file=query.txt (9451 queries)

Benchmark enabling Hot Spot Cache with unlimited slots 
Processed 18902 queries in 82.609620s (228.81qps)
Benchmark enabling Hot Spot Cache with 10*#queries slots 
Processed 18902 queries in 82.378802s (229.45qps)
Benchmark enabling Hot Spot Cache with #queries/2 slots 
Processed 18902 queries in 84.010641s (225.00qps)
Benchmark disabling Hot Spot Cache
Processed 18902 queries in 104.356003s (181.13qps)

Note that all queries were processed two times, so this should basically be advantageous to hot spot cache. Actually, the result was not so different even if queries weren't repeated. This is probably partly because of the nature of the root zone: most of the response would be a referral to popular top level domains, so even if queries are different many RRsets for the authority and additional sections should be reused frequently.

The size of cache didn't make much difference. This may also be because of the characteristics of the root zone.

According to this test, hot spot cache improves performance in terms of qps about 27%. Personally, I'm not so sure if the gain is worth the additional code complexity. Next time we wanted to optimize something, we should first have a proof that it surely makes substantial improvement.

comment:3 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Summary changed from simple benchmark for b10-auth query handling to review: simple benchmark for b10-auth query handling
  • Total Hours changed from 2.0 to 3.0
  • Type changed from defect to enhancement

comment:4 Changed 9 years ago by jinmei

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:6 follow-ups: Changed 9 years ago by vorner

  • Add Hours to Ticket changed from 0.0 to 1.5
  • Owner changed from vorner to jinmei
  • Total Hours changed from 3.0 to 4.5

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?
  • 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?
  • 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?
  • What these two do when cache is not in use? Is it an error or nop?
  • Benchmarks do not have tests, right? Because they are kind of test themself?
  • 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?

By the way, tested to merge trunk into it (only locally), and seems there are no conflicts and everything works, so this is clean.

comment:7 in reply to: ↑ 2 Changed 9 years ago by each

According to this test, hot spot cache improves performance in terms of qps about 27%. Personally, I'm not so sure if the gain is worth the additional code complexity. Next time we wanted to optimize something, we should first have a proof that it surely makes substantial improvement.

Where is the query.txt file that you used as input for this test? (The one I found in the branch only has three lines in it.)

I did actually benchmark the hotspot cache, using queryperf, at the time I was working on it. I saw QPS improve on the order of 350-400%; I also saw that the elapsed time for individual queries dropped on average from 4-8 milliseconds to less than one. So your results here are very surprising to me.

comment:8 Changed 9 years ago by jreed

From my benchmarks (for bind10-devel-20100812), the hot spot cache was 77% faster (than using -n) when using the sqlite3 data source and up to 135% faster for builtin. (Not a fair comparison, since other changes, but the previous release was around 5% faster versus using -n.) (On a different note, which I will report separately, there is a noticable slowdown when using a built-in zone whenever the LIFESPAN_ flushing is done.)

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

Replying to 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.

comment:10 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 0.45
  • Total Hours changed from 4.5 to 4.95

comment:11 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 in reply to: ↑ 6 Changed 9 years ago by jinmei

Replying to vorner:

Forgot to respond to one more point:

  • 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?

Regarding 'const size_t'. It's because this parameter is "const" in the method, i.e., it's not expected to changed within the method.

Ideally, we can omit this qualifier in the declaration (if that's the point of your question). Unfortunately, however, SunStudio? is broken in this sense and fails to match the method signature if this type of const is missing in the declaration.

(I think we should note this in our coding guideline page.)

comment:13 follow-up: Changed 9 years ago by vorner

  • Add Hours to Ticket changed from 0.0 to 0.25
  • Total Hours changed from 4.95 to 5.2

Thanks for explanations (and, with the const, I asked mostly because it looked odd, but if there's a reason...).

Then I think it can be merged.

comment:14 Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

comment:15 in reply to: ↑ 13 Changed 9 years ago by jinmei

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

Replying to vorner:

Thanks for explanations (and, with the const, I asked mostly because it looked odd, but if there's a reason...).

Then I think it can be merged.

Thanks, committed to trunk. Closing ticket.

Note: See TracTickets for help on using tickets.