BIND 10 #192: Data source hotspot cache

[BIND 10 Development]

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

Comment(by jinmei):

 On the specific point of std::list vs in-house list for LRU-based
 cleaning:

 Replying to [comment:22 each]:

 > > 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.
 >
 > I have now tried it with std::list, but I think it will damage
 performance
 > (unless I've missed some key features of std::list, which is certainly
 > possible).
 >
 > As near as I can tell, to use a std::list, I would have to promote
 entries
 > by to the head of the list by using list::remove() followed by
 > list::push_front().  But list::remove() works by iterating over the list
 > searching for entries with the matching value to remove.

 No, in this case you would use list::erase().

 See the attached patch.  This is what I'd do if I were to use std::list
 for this purpose.  This version doesn't have the performance problem you
 raised.

 > > HotCache::~HotCache(): I don't understand what the for loop does. Does
 this do something correct? Also recall my other comment about why using
 in-house list. If we use std::list, we don't even have to bother to clean
 up these things.
 >
 > I'm pretty sure this is correct, now that I've fixed the for loop.

 Maybe you are as you are the author of the implementation, but look at it
 pretending to be you are a reviewer who happens to take a look at it:

 {{{
     for (node = lru_head_; node != CacheNodePtr(); node = node->next) {
         lru_head_ = node->next;
         if (lru_head_) {
             lru_head_->prev = CacheNodePtr();
         }
     }

     lru_head_ = lru_tail_ = CacheNodePtr();
 }}}

 this is way too far from intuitive logic.  I can guess what this code is
 trying to do, but I need to use my brain heaviy to follow the logic, and
 it made me write a separate test (!CacheTest.cleanup) to confirm it really
 does what it seemingly tries to do.

 As I said, on the other hand, we don't even need the destructor in the
 std::list version (see the patch).  With the properties of STL containers
 and shared pointers, it's obvious there shouldn't be any leak.

 Another example: the in-house version has this code in insert():

 {{{
     if (lru_head_) {
         node->next = lru_head_;
         lru_head_->prev = node;
         lru_head_ = node;
     } else {
         node->next = lru_head_;
         lru_head_ = node;
         lru_tail_ = node;
     }
 }}}

 I can see this code tries to insert the new node to the head of list, but
 it's not super obvious that it really does what it's supposed to do, so as
 a reviewer I'd need to use my brain to follow the in-house logic line by
 line.

 The corresponding code in the std::list version is as follows:

 {{{
     lru_.push_front(node);
 }}}

 Anyone who knows STL should be able to understand what it does instantly,
 and can be sure it actually does what it's expected to do.

 You should be able to see many such contrasts in the patch.

 As I said before, std::list version has its own drawbacks.  For example,
 it would require a bit more memory.  But after writing this version myself
 and seeing the difference, I now see the simplified code logic outweighs
 the overhead very much.  What do you think?

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