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                                             
-------------------------+--------------------------------------------------
Changes (by each):

  * owner:  each => jinmei


Comment:

 > !CacheEntry class
 > !CacheNode class

 By changing the signature of HotCache::retrieve(), I was able to
 refactor both of these into a completely hidden objects in cache.cc,
 I hope this addresses most of your concerns with the code.  I'll also
 update them tomorrow with the suggestions you made for their
 documentation, but I haven't done that yet.

 > QTuple class

 I've removed this class entirely, in favor of isc::dns::Question (which
 has
 had an an less-than operator added to it for use by std::map).

 > We tend to abbreviate things, but isn't it better to name it more
 clearly, e.g., !HotSpotCache

 I personally prefer !HotCache, but it's not particularly important to me.
 I'll change it if you think it's important.

 > Class description: like Cachenode, I'd add more design decisions, non
 trivial points, etc.

 Okay, I've done some of that...

 > As for design decision, I guess we need to consider multi-thread
 considerations.

 Good point.  Documented.

 > Design: I don't think making this as a static is a good idea.

 We discussed this further, and I'm convinced.  HotCache is now
 instantiated
 a member of AuthSrvImpl.

 > Design: now the controversial point:-) I strongly suggest >pimpl be used
 for this class.

 Done.

 > I believe the HotCache? </wiki/HotCache> class is intended to be non-
 copyable. Please explicitly make it so.

 Done.

 > naming: if we follow our method name convention, cache() is better named
 setCacheEntry()

 I had been thinking of the word cache() as a verb, not a noun, but since
 there's now a cache() function in the Query objct now that returns a
 reference to the HotCache, I decided you're right about this.  I chose the
 names addPositive() and addNegative().

 > description about cache(): it should describe what happens if there's
 already an entry that matches the key. Also also be tested.

 Done.

 > 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.

 > HotCache::retrieve(): map::operator[] creates a new entry if it doesn't
 have one for the key. It's probably not necessarily incorrect, but I
 suspect it's error-prone behavior. For example, consider the case where
 the caller continuously calls retrieve() for different non-existent keys.
 I suggest you use map::find() instead.

 Done.

 > HotCache::insert(): why do we need to have a while loop?

 Changed to an if (and an assert()).

 > HotCache::ncache(): the condition of ANY()s isn't documented (and not
 intuitive).

 Documented.

 > please don't hardcode flags (e.g. for the second arg of
 HotCache::cache()), even though it's opaque for the HotCache module

 Not yet done, but I will get to it tomorrow.

 > 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.

 If you prefer, you could think of it as eliminating a class that's no
 longer needed (!NameMatch) and replacing it with one that serves our
 needs better (!ZoneInfo).

 The !NameMatch class is *not* a pristine general-purpose class; its sole
 function, prior to this change, was the one it's still being used for now:
 to iterate through through a list of data sources looking for the one that
 is authoritative for a given name.  There is no other use to which it can
 be put.  It already had several quirks, such as the special-case code for
 rrtype==DS.

 I haven't really changed it, I've just given it a slightly broader remit,
 in order to eliminate unnecessary database searches.  I don't think
 there's
 any simpler way to accomplish this that still makes sense.

 > 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:
 >
 > This is totally irrelevant to hot spot caching, and I saw many such
 changes. They are distracting.

 I don't agree that it's irrelevant--though perhaps it's a little
 tangential.  In the interests of making it easier to review, I can change
 the signature of findClosestEnclosure() back to what it was--but I would
 still want to make this change as soon as possible afterward, because IMHO
 it's a bug waiting to happen.  You specify a class when you create the
 !ZoneInfo object; if you then call findClosestEnclosure() with a
 *different* class, it would cause a confusing failure.  It makes a lot
 more
 sense to preserve the information you used when you created the !ZoneInfo
 object.

 However, if you think it's urgent that split this into multiple separate
 fixes, I can do that.

 For the present, however, what I've done is to fully commit to the change
 I
 had made before: I've changed the NameMatch class to be called ZoneInfo,
 which is clearer and more descriptive.  As I say, however, if you think
 it's necessary to split this into multiple changes, I will attempt it
 tomorrow.

 > 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.

 As I've currently written it, you get access to the !CacheNode in O(log n)
 time via the std::map, and then you can promote it to the head of the list
 in constant time.  With std::list, the promotion would be O(n).

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

 Oops, yes, I had reversed the second and third statements.

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