BIND 10 #1747: refactor auth::Query so it's reusable
BIND 10 Development
do-not-reply at isc.org
Tue Mar 13 13:19:49 UTC 2012
#1747: refactor auth::Query so it's reusable
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: high | Sprint-20120320
Component: | Resolution:
b10-auth | Sensitive: 0
Keywords: | Sub-Project: DNS
Defect Severity: N/A | Estimated Difficulty: 4
Feature Depending on Ticket: auth | Total Hours: 0
performance |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by jelte):
* owner: jelte => jinmei
Comment:
Replying to [comment:7 jinmei]:
> Replying to [comment:5 jelte]:
>
> First, regarding this point:
>
> And we encapsulate the query object before calling process(), and let
> `QueryCleaner` clean it up either on success or exception:
>
The cleaner could also be initialized as the first part of process(). I
did think of such an approach, but as the object of this exercise was to
reduce object allocation I was hesitant to do so (though this one would
probably not be much overhead).
> But realistically it's quite unlikely we see exception from process()
> with the in-memory data source, so if you want to defer it to a
> separate task, it's okay for me. At least as long as the vectors are
> cleared at the beginning of process() there shouldn't be anything
> wrong even if an exception happens.
>
That was my conclusion as well :)
> Comments on the implementation follow:
>
> '''misc'''
>
> - I made a few minor editorial/style fixes to the branch.
Much obliged.
> - do we need a changelog? maybe not because it's mostly an internal
> refactoring, but I'm asking anyway since it still changes the
> interface given in a public header file.
>
I don't think so; the header file is public, but still 'internal' to Auth
only.
> '''higher level points'''
>
> - Query now seems to be copyable. Is that okay? There's probably no
> particular reason it cannot be so, but if we don't have a reason we
> want to make a copy of it, it's probably safer to make it
> noncopyable (with noting these) to avoid making an unexpected copy.
done
> - Why is Query::qname_ (and the qname parameter of process()) a real
> object? This will cause a relatively expensive copy for every call
> to process(). It's also now mutable (while it still doesn't seem to
> have to be so), which makes the code more fragile (we could
> accidentally override it in the middle of process()). If there's no
> particular reason, I'd suggest changing it to 'const Name*'
> process() and initialize() could take 'const Name&' and set qname_
> to its address.
As real objects, they do need to be mutable, since we change them on every
call to process, but they could indeed be pointers (though, as I said in
my notes, I expect the next step in the refactor would be to get rid of
all the old members completely, and only pass them to the methods as
references).
Made them pointers for now.
> - Same for qtype_ and dnssec_. Also, especially for qname_ and
> qtype_, I'd be nervous about their initial values. While I
> understand if we really need to keep a real object there's not many
> choices and sometimes we end up using arbitrary initial value. But
> such ad hoc values often lead to bugs - I basically want to keep
> things "undefined" when they don't have real meaning. Maybe we can
> consider using boost::any (not sure about its performance
> impact)...continue to the next point:
for dnssec_ i don't really see an advantage of using a pointer (just a
bool, and it even has a default). Change qtype_ as well.
> - Considering these, I wonder whether we need to keep the query
> parameters within the Query class. From a quick look they are only
> for the convenience of the support private methods - we could also
> pass these parameters to these methods instead of storing them in
> Query and have the private methods refer to them (right)? If
> passing around many parameters causes readability issue, we can
> think about introducing a helper structure, say QueryParam, which is
> essentially a read only tuple of the query parameters. The Query
> class could either hold it as a pointer (and initialize reset it to
> NULL except while doing process()) or pass that object to private
> methods (then there'll be only one additional parameter). The
> QueryParam can be constructed for every query, so if we need to make
> a copy of Name/RRType we don't have to use placeholder initial
> objects for them.
>
I'd prefer we remove them completely and pass them around to all the
methods that need them; the 'biggest' one is already done (process()), and
the rest should be straightforward. The reason I haven't done so yet was
to keep the diff small, and I intend to make this a smaller followup
ticket. Unless you insist, then I'll do it now :)
> - I guess we might want to trim excess vector capacity in case it
> happen to hold unusually large number of items:
> {{{#!c++
> const size_t MAX_RESERVE_RRSETS = 64; // note: arbitrary choice
> if (answers_.size() > MAX_RESERVE_RRSETS) {
> vector<ConstRRsetPtr> new_answers;
> new_answers.reserve(MAX_RESERVE_RRSETS);
> answers_.swap(new_answers);
> }
> }}}
>
I wonder about that. I do think we want to initially reserve some space
(maybe even much larger than we will ever think necessary, 64 may be a
good choice); but would it ever be worth it to reduce it? This would just
be a reallocation for 3 possible vectors; which may very well be increased
again by the next call to process() (causing yet more reallocation), and
for, IMO, relatively little gain. I'm more worried about reallocations
than vector size, tbh.
I've changed the initialization of query to call reserve(RESERVE_RRSETS).
I'm open to discussion on whether we should reduce their size :)
> '''query.cc'''
>
> - We can remove most of the boost::const_pointer_cast (in fact we can
> consolidate them within `RRsetInserter`). (Of course, the need for
> it itself is bad even if it's used "once" and we should eventually
> solve this, but that's another issue). I believe this fix is
> trivial so I made it to the branch directly.
>
Thanks
> - maybe a matter of taste, but I'd let processDSAtChild() call
> createResponse() internally and keep the caller doing nothing.
> Especially for the case of DELEGATION, since this switch is pretty
> big (which I think would also require refactoring, but that's
> another issue), it makes me nervous to see the break because it's
> not so obvious from the code around the "DELEGATION" case what
> happens after 'break'. Note also that with the current convention
> we'd also have update the documentation in the header:
> {{{#!c++
> /// It returns true if this server has authority of the child;
otherwise
> /// it returns false. In the former case, the caller is expected to
> /// terminate the query processing, because it should have been
completed
> /// within this method.
> }}}
> because the "In the former case,..." is not very accurate any more.
>
ok, done. Noting that this may also be a smallish hint that there is room
for more refactoring-improvements :)
> - This createResponse() doesn't seem to be necessary:
> {{{#!c++
> response_->setHeaderFlag(Message::HEADERFLAG_AA, false);
> response_->setRcode(Rcode::REFUSED());
> createResponse();
> return;
> }}}
>
removed
> - If we copy objects in initialize(), we could just pass references to
> process() and initialize(), then we can save a couple of copies
> (although the compiler may optimize these one-shot copies). But see
> also the design level discussions above.
>
yes, already did this for the changes above.
> '''query.h'''
>
> - I'd say "message" instead of "packet" (my usual suggestion in such a
> context:-)
> {{{#!c++
> /// This will take each RRset collected in answers_, authorities_,
and
> /// additionals_, and add them to their corresponding sections in
> /// the response packet.
> }}}
>
done
> - Maybe "Default constructor" is better than "Empty constructor"?
> {{{#!c++
> /// Empty Constructor.
> ///
> /// Query parameters will be set by the call to process()
> ///
> /// This constructor never throws an exception.
> ///
> }}}
> Also, technically, it could throw in constructing qname_.
>
not anymore :) though of course bad_alloc could be thrown. And if we
ignore that, the note itself could be considered superfluous. Removed it.
--
Ticket URL: <http://bind10.isc.org/ticket/1747#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list