Opened 8 years ago

Closed 8 years ago

#1601 closed task (complete)

pass reference instead of shared pointer within AuthSrv

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20120320
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: auth performance
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 3.78 Internal?: no

Description

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.

Subtickets

Attachments (1)

query_bench.diff (3.0 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 8 years ago by 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.

comment:2 in reply to: ↑ 1 Changed 8 years ago by 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).

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:4 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120306

comment:6 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:7 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to assigned

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

comment:8 Changed 8 years ago by vorner

  • Status changed from assigned to reviewing

comment:9 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:10 in reply to: ↑ 7 Changed 8 years ago by 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.

Changed 8 years ago by jinmei

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 0.4

comment:13 Changed 8 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0.4 to 3.78

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

Note: See TracTickets for help on using tickets.