pass reference instead of shared pointer within AuthSrv

[jinmei]

This is probably marginal, but most of the internal methods used
in AuthSrv? could pass references when they pass shared pointers (not:
not a reference to the shared pointer; real reference to the
underlying object).

Passing shared pointers have some extra cost due to its copy
operation, so this will probably improve performance a bit.

[stephen]

If the code currently uses shared pointers, it is less of a change to pass a reference to the pointer through - just add an ampersand to the argument type in the method signature. (I think we should do this everywhere - although we've discussed this in the past.) However, it is probably clearer to dereference the pointer and pass the reference to the object through to the internal method.

[jinmei]

Replying to stephen:

If the code currently uses shared pointers, it is less of a change to pass a reference to the pointer through - just add an ampersand to the argument type in the method signature. (I think we should do this everywhere - although we've discussed this in the past.) However, it is probably clearer to dereference the pointer and pass the reference to the object through to the internal method.

Yes, we've discussed this before and I was not convinced about the
idea: ​http://bind10.isc.org/ticket/1305#comment:9
(I remembered the discussion, and that's why I explicitly noted that
this was "not a reference to the shared pointer").

I'd certainly be against about starting relying on 'reference to
shared pointers' for performance improvement purposes before
completing more organizational fixes and performing profiling based on
the organizationally optimized version.

The case this ticket mentions is certainly something we can really
avoid using shared pointers in the first place, and the proposal of
this ticket is to do such cleanup (which is, IMO, semantically more
correct without any cost).

[vorner]

It should be ready for review. I first tried to change the interface of the callbacks, but I got completely lost in the resolver code (which needed to be changed as well as the result). So this changes it inside the authoritative processing only.

I don't think this needs a changelog entry, as this does not change any public API. The speedup won't be too large either, I think (but I didn't test it).

[jinmei]

Replying to vorner:

It should be ready for review. I first tried to change the interface of the callbacks, but I got completely lost in the resolver code (which needed to be changed as well as the result). So this changes it inside the authoritative processing only.

I don't think this needs a changelog entry, as this does not change any public API. The speedup won't be too large either, I think (but I didn't test it).

The changes basically look okay. I agree that we should keep the
callback interface intact at least for now. I also agree we don't
need a changelog for it.

I have two minor comments:

• Not related to the ticket, but I noticed we can make one additional cleanup:

       MessageLookup(AuthSrv* srv) : server_(srv) {}
       virtual void operator()(const IOMessage& io_message,
                               MessagePtr message,
  -                            MessagePtr answer_message,
  +                            MessagePtr, // unused in this implementation
                               OutputBufferPtr buffer,
                               DNSServer* server) const
       {
  -        (void) answer_message;
           server_->processMessage(io_message, *message, *buffer, server);
       }
   private:
  

• query_bench can be more sharedptr free. I'm attaching a suggested patch.

I'd leave the decision on these to you (although I believe the first
cleanup is good to make even if not in this ticket). After that
please feel free to merge.

[vorner]

OK, thanks. I included the change, even if it's not really related to the the authoritative server itself.