BIND 10 #1975: implement meta-or-container-of data source
BIND 10 Development
do-not-reply at isc.org
Wed Jun 13 15:47:05 UTC 2012
#1975: implement meta-or-container-of data source
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20120619
medium | Resolution:
Component: data | Sensitive: 0
source | Sub-Project: DNS
Keywords: | Estimated Difficulty: 9
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by vorner):
* owner: vorner => jinmei
Comment:
Replying to [comment:11 jinmei]:
> The minor one: now we have two different classes that have
> "container". It may be confusing, and we might want to rename one of
> them. For example, `DataSourceClientContainer` might be renamed
> `DataSourceClientHolder`, or datasrc::Container might be renamed to
> `DataSource(Client?)List`.
ACK. I changed it to ClientList.
> The substantial one: I'm concerned about the performance impact of
> `ConfigurableContainer::search`, especially due to the dynamic memory
> allocation and release, as this method is expected to be called in a
> performance sensitive path, i.e., auth query handling.
> It may not be that severe as new/delete for a small-size object should
> be relatively efficient, but I'd like to make it as efficient as
> possible from the beginning if we can do it by not making the code too
> ugly. As commented, the reason why we use dynamic allocation is
> because `SearchResult` is not assignable. I can think of a couple of
> ways to avoid that:
> - introduce a compatible assignable structure, only internally used in
> container.cc. the search() method uses it inside it, replacing it
> as it finds a better result, and when it returns the final result,
> construct the public one from the internal structure.
> - introduce accessor methods to `SearchResult`, and maintain the
> corresponding member variables as non const.
I went for the first. But I don't thing it would be any difference ‒ the
compiler could be able to optimize the allocations out completely in this
case.
> Likewise, I'd like to make this condition as lightweight as possible:
> {{{#!cpp
> const uint8_t labels(
>
result.zone_finder->getOrigin().getLabelCount());
> if (labels > candidate->matched_labels_) {
> }}}
> for performance sensitive setup, it's likely there's only one data
> source, and in many cases the search will result in a partial match.
> So it would be nicer if we can make the initial comparison simpler
> (besides, the current implementation assumes the default value of
> matched_labels_ (0) is always smaller than the result of a
> getLabelCount() call. While it's true, relying on such an assumption
> seems to be less robust.) I wonder we might change the code like
> this:
> {{{#!cpp
> case result::PARTIALMATCH:
> if (candiate.datasrc_ == NULL ||
> candidate.matched_labels_ <
> result.zone_finder->getOrigin().getLabelCount()) {
> candiate = SearchResult(info.data_src_,
result.zone_finder,
> labels, false);
> }
> }}}
> This will make the expression a bit more expensive for the second or
> higher iterations, but we'd consider such cases less performance
> sensitive.
How does this help? Notice that the result contains the number of matched
labels as well, so we should call it for the first DS too. Or, do you want
to get rid of that part of result?
Thank you
--
Ticket URL: <http://bind10.isc.org/ticket/1975#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list