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:8 each]:

 > >    - We should really, really, seriously reconsider the current design
 of making some of the class member variables public (from prev to qtype).
 >
 > With qname/qclass/qtype, we've been inconsistent in libdatasrc;
 sometimes they're public member variables, sometimes they're functions.  I
 agree we should pick a way and stick with it.  I've begun to wonder if we
 should move toward using a QTuple struct for this.
 >
 > I had a specific reason for making next and prev public--originally I
 hid them, but there was code in the HotCache class that became much
 simpler if it had access to them.  (This could be done by making them
 protected and giving HotCache friend status, but we have avoided using
 friend so far.  Or it could be done by having next_ and prev_ be private
 but exposed via next() and prev() accessor functions.  But as this is
 CacheNode, and next and prev both point to CacheNode and always will, it
 seemed unlikely to me that there would ever be any implementation details
 that would need to be hidden inside those accessor functions.  I'm okay
 with doing it that way if you wish, though.

 In public APIs any unlikely things will happen by some user.  So, yes, I
 insist we should hide them.

 Note, however, that we could adopt pimpl for CacheNode and hide the
 implementation details within .cc.  That way I'm okay with handling them
 directly (I myself take that approach sometimes).

 > >   - document operator<.  why we need this etc.  note also that
 Name::operator< is quite expensive (it calls the generic compare()
 method).
 >
 > Can it be made cheaper?

 Perhaps.  MessageRenderer implements a bit simpler version of comparison,
 but I must say that might be a premature optimization.  This comment is
 actually a heads-up rather than suggesting something else (one of the
 disadvantages of operator overloading, which is they can make expensive
 operations look cheaper).  Before trying to tweak this, we should perform
 benchmark first.

 > >   - retrieve() signature: why passing object copies?
 >
 > If you see me do that and don't know why I did it, the answer is nearly
 always going to be "I forgot the &" and nothing more.  Unfortunately the
 C++ compiler isn't capable of telling me when I've made that mistake, and
 I make it a lot.

 I see.  btw, making classes non copyable whenever possible helps avoid it.
 that's one of the reasons why it's a good idea.  unfortunately, however,
 in this specific case these arguments objects are supposed to be copyable
 and we can't exploit this technique.

 > >   - LRU list: why using in-house list, rather than std::list? (see
 also the comment about the implementation).
 >
 > It just seemed more straightforward to roll my own type with built-in
 promotion-on-access and LRU-cleanup behavior, rather than try to backfill
 that behavior into the std::list type.

 Admittedly, I expect the use of std::list will have its own negative
 implications in this case.  But you seem to underestimate the risk of in-
 house based development with such reasons as "because it seems more
 straightforward".  In house list implementation requires us to write non
 trivial manipulation code like this:
 {{{
     if (lru_head_) {
         node->next = lru_head_;
         lru_head_->prev = node;
         lru_head_ = node;
     } else {
 }}}

 which is not so trivial as the original developer might believe, and we in
 fact often make bugs in the handmade logic.  In fact, you probably
 actually made one (see the ~HotCache() comment below).

 So, while I don't insist std::list is definitely better in this case, I
 would suggest you at least try a version with std::list, not just making a
 decision based on seeming observation.  If, after playing with both ways,
 you are still sure the advantage of in-house implementation outweighs its
 negative consequences, it's your call.

 > > cache.cc
 > >  - HotCache::~HotCache(): I don't understand what the for loop does.
 >
 > Hm.  Not sure this is still needed, it may be left over from an earlier
 code iteration.  But what it does is successively remove references to
 each item on the list so that shared_ptr will deallocate them.

 One thing I cannot understand is the loop condition:
 {{{
     for (node = lru_head_; node = node->next; node != CacheNodePtr()) {
 }}}

 At least the third statement doesn't make sense.  I'm also not sure if
 this loop ever stops because node doesn't seem to be changed in the loop.

 Note also that if you used an STL container you wouldn't have to be
 bothered with these things.  Objects remaining in the container will
 automatically cleaned up when the container is destructed.

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