BIND 10 #192: Data source hotspot cache

[BIND 10 Development]

#192: Data source hotspot cache
-------------------------+--------------------------------------------------
 Reporter:  each         |        Owner:  each                                          
     Type:  enhancement  |       Status:  reviewing                                     
 Priority:  major        |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  b10-auth     |   Resolution:                                                
 Keywords:               |    Sensitive:  0                                             
-------------------------+--------------------------------------------------

Comment(by jinmei):

 Replying to [comment:16 each]:

 > > If we really need to encapsulate the compound logic (on which I'm not
 sure, especially in the context of hot spot cache patch)
 >
 > I believe we do.  Otherwise every query still requires at least one
 database access to find the concrete data source before anything else can
 happen.  That one change, keeping track of the data source in which the
 best enclosing zone had been found, massively improved the query
 performance.

 I see the effect of checking the cache before trying to identify the best
 data source.  But that doesn't mean you have to alter the definition and
 interfaces of an existing class (!NameMatch), affecting many other parts
 of code that have nothing to do with hot spot caching.  That was my point.

 For example, you changed NameMatch, the signature of
 DataSRc::findClosestEnclosure() has also changed, affecting many other
 data source implementations and unit tests.  One specific case is this:
 {{{
  void
 -TestDataSrc::findClosestEnclosure(NameMatch& match,
 -                                  const RRClass& qclass) const {
 +TestDataSrc::findClosestEnclosure(NameMatch& match) const {
      const Name& qname = match.qname();
      NameComparisonResult::NameRelation cmp;

 -    if (qclass != getClass() && qclass != RRClass::ANY()) {
 +    if (match.qclass() != getClass() && match.qclass() != RRClass::ANY())
 {
          return;
      }
 }}}

 This is totally irrelevant to hot spot caching, and I saw many such
 changes.  They are distracting.

 We could (and IMO should) only tweak the code logic so that cache is tried
 before real data sources without (or with minimally) modifying public
 classes.

 Then, the introduction of the cache suggests refactoring the public
 interfaces (which I'm not convinced but we can discuss it), do that in a
 separate task as a pure refactoring.

 I must confess when I'm in the developer side asking for review I'm not
 very good at it either, but the developer is often blind to how confusing
 it is for reviewers because the developer already understands the whole
 picture and everything tends to look so natural.

 Am I now clearer (and hopefully more convincing)?  If so, I'm rather happy
 to make a counter proposal by simplying the data source part of the patch
 with a minimal change.  Possibly rebasing the branch also (as we missed
 the previous release this branch is quite different from the latest
 trunk).

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