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 each):

 Replying to [comment:6 jinmei]:
 > Review is ongoing, but I'll make my comments cache.{cc,h} with a couple
 of general comments first.

 Thank you, as always, for the very thorough review.  (But if you're not
 finished yet, I'm not sure why you assigned the ticket to me at this
 point.  Is there more coming?  If so I'd rather you held the ticket; I'm
 busy on BIND 9 this week and would rather address your comments all at
 once than iterate repeatedly.)

 I'm fine with most of your suggestions.  In particular, in the case of the
 HotCache class, I have no objection to using pimpl.  I simply didn't do it
 on the first pass, because I had a week allotted to coding and wanted to
 avoid adding code complexity while I got the proof-of-concept working.

 A few individual remarks and explanations below.

 >  - Also, I suggest you actually convert the doxygen markups into HTML
 files and browse the results.

 (General question to anyone reading:  How do you do this?  I really have
 no idea.)

 >    - especially if this can be non copyable, I suggest you consider
 using pimpl for this class to reduce dependency on the DNS related class
 implementations in the header file.

 For this class I'm less enthusiastic about pimpl, for reasons you already
 alluded to, but I'll consider it.

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

 >  - QTuple class
 >   - On the other hand, if we really need a public class like QTuple,
 dns::Question is almost exactly what's needed here.  Why not reusing it?
 (we could add operator< if that's the problem).

 Because I didn't remember its existence.  It should be fine.

 >   - Constructor: why passing object copies (which is expensive), rather
 than references?

 Just a mistake.

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

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

 I am open to suggestions.  But we want every query to have access to the
 same cache that all the other queries used.  (And perhaps also every
 update, etc.)  How do we achieve this without making it global, or
 effectively so?  Every solution I thought of seemed to have essentially
 the same problems as the one I chose.

 > One practical reason for this is that we may actually want to have
 multiple different caches if/when we support views.

 Perhaps, but then we'll be wanting to

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

 >   - setSlots description: it states "up to the number of RRsets in the
 data source".  I don't understand this.  Which data source is it?
 Whatever it means, since there can be negative cache entries, this
 description still doesn't make sense to me.

 Yeah, doesn't make sense to me either.  I was probably stoned or
 something. :)

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

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

 >  - HotCache::insert(): why do we need to have a while loop?
 >    if count_ > slots_ > 0, count_ must be slots_ + 1, and we should
 >    only need a single remove operation.  if this doesn't hold it's an
 >    internal bug, and I'd rather let it fail with assert (which I
 >    prefer) or exception than preparing for the "impossible case" with
 >    a complicated logic.

 I was thinking that slots_ might have been modified recently, but actually
 setSlots() takes care of that case, so perhaps you're right.

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