BIND 10 #2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version

[BIND 10 Development]

#2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I think this ticket can now be closed.

 As you know I've already merged this branch with other remaining
 branches for the feature.  So please simply remove the branch on
 closing the ticket.

 As discussed on jabber, I made some simplification and style cleanups.

 Responses to some specific points in the previous discussion follow:

 Replying to [comment:17 muks]:

 > > '''memory_client.cc'''
 > >
 > > - what's the purpose of the changes at
 > >   e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
 > > {{{
 > >     [2218] Call setSigned(true) when NSEC3Data is set
 > > }}}
 > >   The commit log doesn't explain the reason, and no test fails even
 > >   without the changes.  It's probably more about the semantics of "is
 > >   signed" in general - from a quick look, we don't seem to have a
 consistent
 > >   interpretation between the zone finder, (document on possible
 > >   implementation of) zone data, and your implementation of the memory
 > >   client.  Maybe it's a subject of a separate ticket.  But in any
 > >   event, if you want to introduce a change like this (that changes the
 > >   state of the data), I think there should be some corresponding test
 case that
 > >   would fail without the change.  So we must either provide a test or
 > >   defer the change to the later ticket.
 >
 > From a reading of ZoneData's api doc, I thought `setSigned()` would also
 be called with `true` whenever NSEC3 data existed. I've added tests to
 assert this.

 I'm not sure where in the zone data doc you had the impression, but
 according to this:
 {{{#!cpp
 /// This class does not actually relate the status of signed-or-not to
 /// any of its other attributes; it's up to the application how to set or
 /// use this status and maintain it in a reasonable way.  One possible
 /// definition is to set this status if and only if the zone has a
 /// DNSKEY RR at the zone origin (which is BIND 9's definition of signed
 /// zone).  When the application adopts this definition, it's the
 /// application's responsibility to keep the status consistent with the
 /// actual existence or non-existence of a DNSKEY RR.
 ....
 /// Note that even though \c isNSEC3Signed() being true should indicate
 /// \c isSigned() is true too in practice, the interfaces do not
 /// automatically ensure that, so we'd need to check both conditions
 /// explicitly.  And, in fact, if we adopt the above definition of
 /// \c isSigned(), it's possible that a zone has a complete set of NSEC3
 /// RRs but no DNSKEY (although it's effectively a broken zone unless we
 /// support incremental signing).
 }}}

 It rather suggests the concept of signed-or-non-signed is independent
 from the existence of NSEC/NSEC3. (but maybe the second paragraph gave
 you your impression?).

 The relationship and clear definition of "signed" were left open
 partially on purpose - I expected when we support incremental
 signing or migration between NSEC and NSEC3, it will be possible that
 the zone wouldn't be considered "signed" but still have NSEC or NSEC3
 records.  That's the basic rationale of separating the concept of
 "NSEC signed" and "zone is signed".  In that sense, it doesn't really
 match the original vague intent if we mark the zone "signed" just
 because the zone has NSEC3 (or NSEC3PARAM).

 But, anyway, at the moment this won't cause any difference in
 observable server behavior as we only support complete, full loading
 of zones.

 So, for the purpose of this ticket I'm okay with the current code.
 We'll need a separate cleanup ticket though.

 > > '''zone_finder_unittest.cc'''
 > >
 > > - does addZoneData() need everything for the NSEC3 case?  It seems to
 > >   be almost a duplicate of the main code, which is not generally good.
 > >   For example, if the test data aren't expected to contain
 > >   inconsistent NSEC3 param, it's probably better just to skip the
 > >   check.
 >
 > Let's keep this for now as the worst thing it does is check more. :) I
 want to finish up the other loose ends and merge this.

 Okay, can you open a ticket for that?

 > > '''zone_finder.cc'''
 > >
 > > I have a few more suggestions
 > > - InMemoryZoneFinder::getOrigin() is expensive and should better be
 > >   avoided whenever possible.  It looks like possible to use the label
 > >   sequence of the origin node and use LabelSequence based interfaces
 > >   in both cases.
 >
 > Done. I've also made it use the previous-chain relative `find()` which
 was added in a recent bug to search relative to the origin node.

 Okay.  In the case of the NSEC3 tree I'm not sure how much it buys or
 whether it's even really faster (I'm assuming you did it as a kind of
 performance optimization), but I don't oppose to the change itself.

 Looking into the code further, I noticed another possibility of
 optimization:
 {{{#!cpp
     for (unsigned int labels = qlabels; labels >= olabels; --labels) {
         const Name& hname = (labels == qlabels ?
                              name : name.split(qlabels - labels, labels));
         const std::string hlabel = hash->calculate(hname);
 }}}
 if we extend `NSEC3Hash()` so it can take a `LabelSequence` (must be
 absolute), we should be able to avoid the expensive `Name::split()`.
 But that's definitely beyond the scope of this ticket.  And,
 calculate() is probably the most significant bottleneck in this
 context anyway, so other optimizations may be marginal.

 > > '''nsec3hash.h,cc'''
 > >
 > > - A minor, and possibly controversial point: for the new signature of
 > >   factory, it might be better to take `const uint8_t*` and `size_t`
 > >   instead of `vector<uint8_t>`, considering this is a relatively lower
 > >   level primitive when performance is important.  If we require a
 > >   vector, the caller needs to prepare for it, which might be expensive
 > >   depending on the context (see above).
 >
 > It's been changed now. `NSEC3Hash` was internally refactored for it.

 I'd keep the internal vector and copy the given data + size into it,
 but I don't oppose to your version.

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