Opened 8 years ago

Closed 8 years ago

#1747 closed task (complete)

refactor auth::Query so it's reusable

Reported by: jinmei Owned by: jelte
Priority: high 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: 4 Add Hours to Ticket: 0
Total Hours: 12.66 Internal?: no

Description (last modified by jinmei)

The auth::Query class is now (going to) use internal resource
to hold RRsets with #1607. In #1607 I used a local
vector<ConstRRsetPtr> object, but it's not efficient to allocate it
for every query.

I propose making auth::Query reusable and reusing the internal
resource within the class. Then the auth server creates a Query
object on its initialization and keeps using it. Make sure the
internal resource is always cleared for every query, especially when
some query processing results in an exception. It would also be
better to ensure the capacity of the vector(s) won't exceed some
limit after cleanup.

This would be a relatively easy task, so I'd also make one more
refactoring in this ticket: using separate vectors for the answer and
authority sections and storing RRsets for these sections in the middle
of the query processing. Then we can conslidate the response message
construction at the end of the method:

    for_each(answers.begin(), answers.end(),
             RRsetInserter(response_, Message::SECTION_ANSWER, dnssec_));
    for_each(authorities.begin(), authorities.end(),
             RRsetInserter(response_, Message::SECTION_AUTHORITY, dnssec_));
    for_each(additionals.begin(), additionals.end(),
             RRsetInserter(response_, Message::SECTION_ADDITIONAL,
                           dnssec_));

which would be clearer, and will make #1688 easier to handle.

It's not direct dependency, but it's probably better to start this
task after #1607.

Subtickets

Change History (16)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:3 Changed 8 years ago by jelte

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

comment:4 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ok ready for review,

I've moved the constructor arguments to process(), which uses a new internal method initialize() to set the members, which are now non-const and pointers instead of references (though the method arguments are still refs so as not to risk nullpointers).

I moved the rrsetptr vectors out of process(), they are now members as well, and split up target_ into answers_ and authorities_.

They are not put into the response until createResponse() is called (done by process() before it returns).

Regarding these vectors; process() is not exception-safe (just like it was not and is not safe because it modifies response); they can be modified. However, these vectors are reset both in createResponse and before process() starts to do any work, which i think for now is good enough.

I also added a lettuce test that does multiple queries, to make sure it also works there.

Due to the changed API, the diff got quite big, esp. for the unit tests, but most of these changes are straightforward.

There is also future refactoring work we could do; I think we could probably get rid of the qname/qtype/datasrc/etc members altogether, and only pass them along as references. We can also make process keep track of everything, and not modify response until the very end. And there's probably more :p

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

First, regarding this point:

Regarding these vectors; process() is not exception-safe (just like it was not and is not safe because it modifies response); they can be modified. However, these vectors are reset both in createResponse and before process() starts to do any work, which i think for now is good enough.

In my definition of exception safety it's safe in that it doesn't
cause such disruption as memory leak on exception; but it doesn't
provide the strong exception guarantee (either complete success or
fall back to the initial state on exception). I interpret you meant
this, and that's true. And, in fact, it's probably almost impossible
to provide the strong guarantee for the response message (unless we
prepare a temporary copy beforehand, which would be too costly), so I
think it's acceptable.

Regarding the internal buffers, I think it's relatively easy to
guarantee that: We can use a small helper "scope" object

class QueryCleaner {
public:
    QueryCleaner(auth::Query& query) : query_(query) {}
    ~QueryCleaner() { query_.reset(); } // we need to make reset public
private:
    auth::Query& query_;
};

And we encapsulate the query object before calling process(), and let
QueryCleaner clean it up either on success or exception:

        if (memory_client_ && memory_client_class_ == question->getClass()) {
            const RRType& qtype = question->getType();
            const Name& qname = question->getName();
            QueryCleaner cleaner(query_);
            query_.process(*memory_client_, qname, qtype, message, dnssec_ok);
        } else {

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.

Comments on the implementation follow:

misc

  • I made a few minor editorial/style fixes to the branch.
  • 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.

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.
  • 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.
  • 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:
  • 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 guess we might want to trim excess vector capacity in case it happen to hold unusually large number of items:
    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);
    }
    

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.
  • 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:
        /// 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.
  • This createResponse() doesn't seem to be necessary:
            response_->setHeaderFlag(Message::HEADERFLAG_AA, false);
            response_->setRcode(Rcode::REFUSED());
            createResponse();
            return;
    
  • 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.

query.h

  • I'd say "message" instead of "packet" (my usual suggestion in such a context:-)
        /// This will take each RRset collected in answers_, authorities_, and
        /// additionals_, and add them to their corresponding sections in
        /// the response packet.
    
  • Maybe "Default constructor" is better than "Empty constructor"?
        /// 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_.

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to 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:
    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:
        /// 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:
            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:-)
        /// 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"?
        /// 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.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

The revised version looks basically okay and ready for merge. One
final suggestion: I'd reset datasrc_client_/qname_/qtype_/response_ to
NULL in reset().

Replying to jinmei:

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

The construction overhead of this QueryCleaner object should be
essentially nothing more than this:

Query& query_ref = query_;

Anyway,

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

so I'm okay with deferring it. Would you open a ticket for that?

  • I guess we might want to trim excess vector capacity in case it happen to hold unusually large number of items:
    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 :)

As long as the reserved space is sufficient, reallocation shouldn't
happen run-time, so unless we repeatedly build a response containing a
crazy number of RRsets, reallocation shouldn't happen again so soon.
But it's probably less likely to happen in the first place anyway (we
could have a crazy number of *RRs*, but it would be very unlikely to
include a large number of *RRsets*). So I'm okay with skipping this
cleanup. At least I'm okay with deferring it.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 2.66

comment:13 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

The revised version looks basically okay and ready for merge. One
final suggestion: I'd reset datasrc_client_/qname_/qtype_/response_ to
NULL in reset().

ack, done

Replying to jinmei:

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

The construction overhead of this QueryCleaner object should be
essentially nothing more than this:

Query& query_ref = query_;

Anyway,
so I'm okay with deferring it. Would you open a ticket for that?

crap it, that would almost have been more work than actually doing it :p

Ok, added a private class QueryCleaner?; which is initialized first thing in process(). Removed the other calls to vector::clear() and query::reset(). The multi-query tests should already show that this cleaner class is used and works (not sure whether it is worth it adding other methods to inspect the cleaner object is used).

As long as the reserved space is sufficient, reallocation shouldn't
happen run-time, so unless we repeatedly build a response containing a
crazy number of RRsets, reallocation shouldn't happen again so soon.
But it's probably less likely to happen in the first place anyway (we
could have a crazy number of *RRs*, but it would be very unlikely to
include a large number of *RRsets*). So I'm okay with skipping this
cleanup. At least I'm okay with deferring it.

created ticket #1777

Passing back for a final look at those changes :)

comment:14 in reply to: ↑ 13 Changed 8 years ago by jinmei

Replying to jelte:

crap it, that would almost have been more work than actually doing it :p

Ok, added a private class QueryCleaner?; which is initialized first thing in process(). Removed the other calls to vector::clear() and query::reset(). The multi-query tests should already show that this cleaner class is used and works (not sure whether it is worth it adding other methods to inspect the cleaner object is used).

Looks okay. To be perfect I might want to add one test case (in
addition to checking two successful queries) where an
exception is actually thrown in process, which shouldn't leave garbage
in the response to the next query. But the setup for it may be
complicated, and it's mostly obvious from the code that it should do
the cleaning. So I'm okay with skipping it. Please merge.

comment:15 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:16 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 2.66 to 12.66

Thanks, merged, closing ticket.

Note: See TracTickets for help on using tickets.