BIND 10 #1305: auth NSEC support: some more updates to data source

[BIND 10 Development]

#1305: auth NSEC support: some more updates to data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111025
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 stephen]:
 > > Do you mean passing as !DatabaseAccessor?& or as
 boost::shared_ptr<!DatabaseAccessor?>&?
 > The latter.
 >
 > > In the latter case we'll have to dereference it to acquire the
 ownership anyway for the same reason, so I suspect the actual benefit of
 saving the copy is marginal
 > Well, there is work to do in incrementing and decrementing a reference
 count.  If passed by value, a copy is made within the function (causing an
 increment of the reference count) then that copy is passed as argument to
 the accessor_ copy constructor.  Finally, the copy is discarded (a
 decrement of the reference count).  You are right in the the cost saving
 is marginal, but we use shared pointers extensively in BIND 10 and all the
 costs mount up.
 >
 > > (in this specific usage, in particular, I guess the compiler will
 combine the multiple copy constructions involved in the function call and
 the member variable initialization, and the actual benefit would actually
 be zero).
 > Bear in mind that a copy constructor could contain code unrelated to the
 copy (which may be a bad idea but is not prohibited by the C++ standard),
 so if the compiler were to do that it could be break required behaviour.
 However, since the boost code is mainly headers, it could be that the
 compiler is clever enough to see that there are no external calls and to
 optimise the copy away.  But we don't know that.

 I don't deny your points, but in terms of performance considerations
 I'd say

 - even dereferencing of a reference of a shared pointer is costly if
   we care about the marginal overhead of copying a shared pointer
   object because it involves a function call.  So, if the overhead of
   method calls on a shared pointer object really matters (there may be
   some such cases like in the middle packet processing), I'd rather
   pass a reference to the pointed object at the higher level of code
   (in this example, it would be !DatabaseAccessor&).
 - I'd generally rather avoid premature optimizations.  I don't object
   to making this level of optimization when we are at the stage of
   performance tuning and if we know the accumulated overhead around
   shared pointers is a real bottleneck via benchmarking.  But it's not
   now.
 - In any case, this specific context isn't performance sensitive at
   all - the accessor is expected to be created during initialization
   of the server and will be used for quite a long period.  In some
   cases we'll also use ephemeral accessors, but in that case the
   performance wouldn't matter much anyway.

 But at the same time, I agree we tend to use shared pointers even when
 they don't have to be so.  I don't think it good, not necessarily for
 performance, but rather due to other disadvantages of shared pointers
 such as making it unclear who is actually responsible for the
 ownership of the object.  So, as a matter of cleanup I think it a good
 idea to revisit our use of shared pointers.  But that will be a rather
 larger task outside the scope of this ticket (and, as you noticed,
 this interface is not actually part of this branch).

 (In this particular, IMO the best would be to have the factory
 maintain the accessor in the form of an auto_ptr and pass
 it as in that form or bare pointer, and let DatabaseClient maintain it
 in the form of scoped pointer.  This way we handle the things safely
 with minimally powerful tool.  But auto_ptrs are often error prone
 also, so it may not be a good practice in the end either...)

 > > In general, I'd be careful about passing a shared pointer in the form
 of reference because it could often lead to break the "shared" nature of
 the object. If the called function handles the passed reference carelessly
 (e.g., keep storing it somewhere in the form of reference) it can easily
 lead to dangling ownership. But such a discussion would be far beyond the
 scope of this ticket.
 > I'm not sure I follow your line of reasoning here.  A function keeping a
 pointer (or reference) to a passed-in object if there is no guarantee that
 the object will remain in existence when the pointer/reference is next
 used is a problem regardless of the object being passed in.  In this case,
 there is no guarantee about the lifetime of the shared_ptr object (other
 than it will exist for the duration of the call to this method), so the
 function shouldn't store a pointer or reference to it.

 My point is that passing a reference is more error prone, and the
 mixture of (effectively) passing a shared pointer (which implicitly
 indicates the object is assumed to be shared) and actually passing it
 in the form of reference will increase the risk of errors.  In
 principle, you're right that the fact that something is passed by
 reference indicates the caller controls the ownership, but I'm afraid
 the fact that the referenced object is a shared pointer can give the
 callee a false sense of safety.  This is an exaggerated bad example:

 {{{
 class Foo {
 public:
     Foo(shared_ptr<Bar>& barptr) : barptr_(barptr) {
       // and barptr_ will be used throughout the lifetime of Foo.
     }
 private:
     shared_ptr<Bar>& barptr_;
 };
 }}}

 but I can imagine a more subtle, but essentially the same, bugs like
 this.  As I said above, if we are sure that the caller should have the
 ownership of the original object, I'd rather pass a reference (or,
 suboptimally a pointer if it can be NULL) of the object that the
 shared pointer points to, not a reference of the shared pointer; if
 the ownership is expected to be shared, I'd rather pass it as a real
 shared pointer to clarify the intent, rather than hoping that the
 callee understands the intent and behaves accordingly.  And,
 personally, I don't like to sacrifice the clarity of the intent for
 possible performance reasons unless we know the performance bottleneck
 is real and substantial in our main usage.

 > I'll leave it up to you whether you change the method signature.  The
 code is OK to merge.

 So...for various reasons I won't touch this part and merge the rest of
 the code for now.  Again, I don't mind revisiting how to use shared
 pointer throughout of our code.  If you want please create a separate
 ticket or trigger a discussion at the dev list.

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