Opened 8 years ago

Closed 6 years ago

#1894 closed enhancement (fixed)

use labelsequence in findNSEC3

Reported by: jelte Owned by: muks
Priority: high Milestone: bind10-1.2-release-freeze
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by shane)

in findNSEC3 (both for db and mem backends) we use Name::split() a number of times, before hashing the result. We could use the LabelSequence class for this, instead of the copying Name::split() call.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jelte

note, this probably means the hash calculator needs to be a bit more flexible in its input arguments (but i have not checked)

comment:2 Changed 8 years ago by shane

  • Milestone New Tasks deleted

comment:3 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Owner set to UnAssigned
  • Status changed from new to reviewing

Up for review. The Name variant of calculate() was also optimized (this was necessary anyway to reuse the same code in the LabelSequence variant of calculate() that was added later.

comment:4 Changed 6 years ago by muks

No ChangeLog entry is necessary as this is a performance optimization only.

comment:5 Changed 6 years ago by shane

  • Description modified (diff)

comment:6 Changed 6 years ago by shane

  • Priority changed from medium to high

comment:7 follow-up: Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

This seems to work. However, it could be yet more efficient if we didn't have to repeatedly copy and downcase the name - e.g. if we could hand calculate() a Name or LabelSequence that was already downcased. But that's a minor optimization, and probably out of scope for this ticket.

Of much greater concern is that the Name::split() technique is still used in DatabaseClient::Finder::findNSEC3(), and the ticket description explicitly says "for db and mem backends".

comment:8 in reply to: ↑ 7 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

Hi Paul

Replying to pselkirk:

This seems to work. However, it could be yet more efficient if we didn't have to repeatedly copy and downcase the name - e.g. if we could hand calculate() a Name or LabelSequence that was already downcased. But that's a minor optimization, and probably out of scope for this ticket.

This is a good idea. I've created ticket #3321 for it.

Of much greater concern is that the Name::split() technique is still used in DatabaseClient::Finder::findNSEC3(), and the ticket description explicitly says "for db and mem backends".

Good catch. I forgot to fix the database backend. It's done now. Please review.

comment:9 Changed 6 years ago by pselkirk

  • Owner changed from pselkirk to muks

Looks good. Please merge.

comment:10 Changed 6 years ago by muks

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

Merged to master branch in commit fe7ae0a7b3eca38b81779b9cad22161e4bcf1d26:

* 7c1e2ec [1894] Minor coding style fix
* f2ce3de [1894] Avoid doing costly name splits (database backend)
* 498b511 [1894] Add missing method
* aff22ac [1894] Use a single LabelSequence object instead of doing costly name splits
* 4aef41d [1894] Add LabelSequence version of NSEC3Hash::calculate()
* e29050c [1894] Avoid expensive Name copy

There was a merge conflict, but the exact code that was the result of the resolution has been reviewed in this ticket.

Resolving as fixed. Thank you for the reviews Paul.

Note: See TracTickets for help on using tickets.