BIND 10 #2701: DHCPv6 Performance Testing and Enhancement
BIND 10 Development
do-not-reply at isc.org
Tue Mar 5 17:13:16 UTC 2013
#2701: DHCPv6 Performance Testing and Enhancement
-------------------------------------+-------------------------------------
Reporter: marcin | Owner: tmark
Type: task | Status:
Priority: medium | reviewing
Component: dhcp6 | Milestone:
Keywords: | Sprint-DHCP-20130328
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
Replying to [comment:4 tmark]:
> Removal of the call to validate() from OptionDefiniton::optionFactory()
> looks safe. Good catch.
>
> One thought, would there be any value in adding a with_validate
parameter
> (defaults to false/no) to OptionDefinition::optionFactory() and then
performing
> the validate only when the flag is true/yes?
I wouldn't bother that. There are two places in the code where the
!OptionDefinition objects are created: in the libdhcp++ to create
definitions for standard DHCP options, in the Config Manager when the
configuration is committed. In the first case, definitions are created
statically and will never change. In the latter case, the definitions may
change only when the new configuration is committed. This is always
handled by the Config Manager which explicitly calls
!OptionDefinition::validate() function when the new definition is added by
a user and rejects configuration if any of the definitions is invalid.
This implies that the !OptionDefinition is always valid when committed
successfully. Thus, I don't see any reason to why we should be validating
definitions between configuration commits.
>
>
--------------------------------------------------------------------------------
>
> Question on on the memfile composite indexes - did you do monitor memory
usage
> as well as CPU usage? While boost is typically very good about memory
management
> I am a little curious to know how much impact on memory usage this had,
and to
> ensure it is leak-free.
I didn't for a couple of reasons:
- I have had been using multi_index_container (and composite indexes)
elsewhere in the code already and never spotted any memory leaks.
- In the BIND10 build farm, there is at least one system that runs the
Valgrind Memcheck for each commit. I believe it would have been caught
already if there was a memory leak in boost.
- We are not oriented to test external libraries. However, it happens from
time to time that someone discovers bug in boost or other external
software and reports a bug to the maintainer.
I limited performance tests to checking server's throughput and CPU
utilization and to finding the correlation between both. Checking the
memory utilization is a good idea and could be included in the next round
of performance testing. IMO, this should not be something that we do
specifically to check the memory management efficiency of the
multi_index_container but rather the general type of test that checks
whether there are no obvious places in the code where we waste the memory.
>
> On a more minor level, the KeyFromKey naming could perhaps be clearer.
>
> You aren't really extracting one key from another key, you are
extracting a "key"
> from within an object hierarchy. So perhaps KeyFromNestedObject or even
just
> KeyFrom might be better. Rather than pass in KeyExtractor1 and
KeyExtractor2,
> it might be clearer to name them something like KeyExtractor and
> ObjectExtractor:
>
> template<typename KeyExtractor, typename ObjectExtractor>
> class KeyFrom {
I got the idea of cascaded extractor from the boost reference:
http://www.boost.org/doc/libs/1_53_0/libs/multi_index/example/complex_structs.cpp.
They called this class key_from_key and I thought it was a good idea to
preserve the name. For someone familiar with boost reference (specifically
with the cascaded extractors) what this class is doing.
However, the new class is actually an extractor. It simply wraps two
extractors specified as template parameters and externally is used exactly
in the same way as standard extractors it wraps. So, I thought it might be
good to add !''Extractor!'' postfix (which I did) to the name of the class
to underline that it is just another extractor class.
>
>
> Perhaps a little more clarity on the parameter comments themselves would
be
> sufficient, for example:
>
> /// @tparam KeyExtractor1 extractor used to access the object used
as
> /// a key from the other object that holds it.
>
> An alternative "...used to extract the key value from the object
containing it"
>
> /// @tparam KeyExtractor2 extractor used to access data in the
object
> /// holding nested object used as a key.
>
> An alternative "... used to extract the nested object containing the
key"
>
Good catch. I changed that as suggested. Also, I updated the description
of the !KeyFromKeyExtractor class to make it (as I think) a bit clearer.
>
>
--------------------------------------------------------------------------------
>
> On commit e0c71421f186a8cf77f1bf7aba7aff072a9caff7, comment says you
added a
> const, but diff looks like you removed one.
True, but there is nothing I can do about it now. :-)
>
>
--------------------------------------------------------------------------------
>
> Regarding, the documentation changes and new tests under dhcp-val, only
one
> comment:
>
> There is a cfg file for memfile testing but it is not mentioned as a
test case
> in v6.performance.1.txt nor is there a separate test description for it.
I added this test case for either V4 or V6.
>
>
--------------------------------------------------------------------------------
>
> The types of mysql server tuning parameters discussed in url,while
interesting,
> fall into the category of site-specific/dba managed values. Unless they
can be
> specified on per session, or per database basis we risk undermining some
other
> use of mysql on the system.
Agreed. The goal of the test was to check the performance in various
conditions, settings etc. It is not necessarily recommended for everyone
to use the settings that allow the highest throughput if there are other
restrictions that do not permit that on the particular environment.
Perhaps, it is worth to add a section describing drawbacks of using
various settings in the report doc, which is summarizing the performance
testing effort for both Kea and DHCP4? This document has been shared with
the Team a few days ago. If possible, please post your comments regarding
this document via email as it may be shared with our customers.
>
> The callgraphs show, as has been discussed before, that we may have some
> opportunities in how we handle the IPv6 addresses, given that we spend
5% of our
> time converting them "fromBytes". If we move to binary storage in
MySQL
> (binary(16)), then we are likely to save both on the conversion we are
doing
> as well as better index/search peformance from MySQL.
Perhaps, this is another thing that we may include in the performance
report document as a suggestion for future experiments.
>
> Overall, this is a very good start at what is far from a trivial effort.
>
>
Thanks.
The proposed !ChangeLog entry for the changes in the Kea code:
{{{
5XX. [func] marcin
b10-dhcp4, b10-dhcp6: Removed unnecesary calls to the function
which
validates option definitions used to create instances of options
being decoded from the received packets. Eliminating these calls
lowered the CPU utilization by the server by approximately 10%.
Also, added the composite search indexes on the container used to
store DHCP leases by Memfile backend. This resulted in the
significant performance rise when using this backend to store
leases.
(Trac #2701, git abc)
}}}
Please review.
--
Ticket URL: <http://bind10.isc.org/ticket/2701#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list