Opened 8 years ago

Closed 6 years ago

#1839 closed task (fixed)

specialize BasicRRset::toWire() for higher performance

Reported by: jinmei Owned by: muks
Priority: high Milestone: Sprint-20131015
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: auth performance
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

According to the benchmark report fro Jeremy, there are still some
cases where BIND 10 is slower than BIND 9. I have an idea of a
relatively low-hanging fruit to make it a little bit faster:
specialize the BasicRRset::toWire() so it exploits the knowledge
of its specific implementation: storing Name/Type/Class?/TTL in
the form of native object and storing RDATAs in a vector.

So the specialized version can avoid the overhead of calling toName()
etc (note that they are virtual so when used in the abstract class it
cannot be inlined), and avoid the overhead of RdataIterator related
overhead.

From a quick experiment it can improve qps of the ./SOA query for 8.4%
(without EDNS) and 13.8% (with EDNS + DO). I believe we can do this
without duplicating much of the code with the generic version.

This optimization may be moot for in-memory because in near future it
will change the internal representation of RRsets even more
customized. But it will still help some other data sources like
SQL-based ones, and the implementation cost should be relatively
cheaper, so I think it's still worth trying.

Subtickets

Attachments (1)

rrset.diff (2.5 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (6)

Changed 8 years ago by jinmei

comment:1 Changed 8 years ago by jinmei

quick experimental version is attached for reference.

comment:2 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Priority changed from medium to high
  • Status changed from new to reviewing

Up for review.

  • Jinmei's patch was adapted
  • Bogus toWire() unittests were fixed that masked problems in code. This bad code was also fixed.
  • toWire() code was also rewritten to use the specialized superclass implementation instead of directly calling rendering code.

Up for review. Raising priority slightly.

comment:3 Changed 6 years ago by muks

I don't think a ChangeLog entry is necessary.

comment:4 Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

This appears to do what it claims, so go ahead and merge.

comment:5 Changed 6 years ago by muks

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

Merged to master branch in commit e3038321406be5d3548edb666de79b63b818dba2:

* 0b20f8d [1839] Fix toWire() for class=ANY and class=NONE cases
* 273e917 [1839] Fix bogus unittests that masked real errors
* 0edfc14 [1839] Update test comments
* 57a2ddc [1839] Ensure that BasicRRset's methods are actually called (cleanup duplication)
* 6a7967f [1839] Fix unittest comment
* 70db20e [1839] Add some comments
* 2b4fea8 [1839] Specialize BasicRRset::toWire() for higher performance

Resolving as fixed. Thank you for the review Paul.

Note: See TracTickets for help on using tickets.