BIND 10 #1959: Implement perfdhcp control logic
BIND 10 Development
do-not-reply at isc.org
Tue Sep 4 09:13:00 UTC 2012
#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
Reporter: | Owner: tomek
stephen | Status: reviewing
Type: | Milestone: Sprint-
enhancement | DHCP-20120917
Priority: | Resolution:
medium | Sensitive: 0
Component: | Sub-Project: DHCP
perfdhcp | Estimated Difficulty: 0
Keywords: | Total Hours: 0
Defect Severity: N/A |
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by marcin):
Replying to [comment:7 tomek]:
> '''tests/tools/perfdhcp/command_options.h'''
> Comment for initialize() method: Initializes class members based [on
the] command line. There are missing dots at the end of many doxygen
comments.
Comments corrected.
>
> Why is the generateDuidPrefix() function contains prefix in its name? It
generates the whole DUID, not just prefix. Prefix has no meaning in the
DUID context. As prefix has specific meaning in DHCP context, it is better
to avoid ambiguity here. Perhaps it would be better to use "template"
instead?
I have taken naming convention from old perfdhcp implementation but after
thinking that through again I admit that "Template" sounds better here.
Functions have been renamed.
>
> generateDuidPrefix() uses 6 byte long MACs. That is ok for now, but it
should be extended eventually. The new DUID type DUID-UUID is more than 14
octets long. RFC3315 states that DUIDs can be up to 128 bytes long. We
should test it one day. I don't suggest that we should
implement it now, but adding some TODO comment is reasonable.
I added \todo comments in the header files.
>
> CommandOptions::printCommandLine() should also print specified command-
line parameters as they are. That may seem redundant, but there is very
good reason to print them out. It is easy to reproduce the case, based
only on log file. It is very useful to keep a format that can be copy-
pasted easily into the next execution.
Good suggestion. Tool now always prints the command line as it was typed
in.
>
> There are no tests for initIsInterface() and generateDuidPrefix().
These functions are not called explicitely but they are called by the
CommandOptions internal logic when it parses command line. Please see
added comments in checkDefaults() and
Interface test.
>
> '''tests/tools/perfdhcp/localized_option.h'''
> Comments for constructors should advise that options 0 and 255 (v4) and
0 (v6) are not valid. It is still very much ok to allow them for server
testing purposes.
I added comments.
>
> Constructors:
> {{{
> LocalizedOption(dhcp::Option::Universe u,
> uint16_t type,
> dhcp::OptionBufferConstIter first,
> dhcp::OptionBufferConstIter last)
> }}}
> and
> {{{
>
> LocalizedOption(dhcp::Option::Universe u,
> uint16_t type,
> dhcp::OptionBufferConstIter first,
> dhcp::OptionBufferConstIter last, const size_t
offset)
> }}}
> can be combined, with the default value of the fifth parameter being 0.
Good catch. I removed redundant definitions.
>
> Invalid namespace specified in line 172. It is isc::perfdhcp, not
perfdhcp.
Corrected.
>
> '''perf_pkt4.cc''' and '''perf_pkt6.cc'''
> Methods writeAt() and writeValueAt are not tested.
I added tests for these methods and .... I found bug in writeValueAt.
Thanks for this! :-)
>
> '''pkt_transform.cc'''
> PktTransform::writeAt() uses inefficient byte-by-byte copying. It would
be faster to use something like
> {{{
> memcpy(&in_buffer[dst_pos], &(*first), distance(first, last));
> }}}
> This is a performance testing tool - using tradeoffs between clean C++
approach vs raw speed should usually favor speed.
Fixed.
>
> I couldn't find any tests for PktTransform class. Are there any? I see
that it is used by PerfPkt4, PerfPkt6 that are tested, but it would be
better to test PktTransform directly as well.
PktTransform does not have any dedicated tests but as you have noticed all
functions are directly called by PerfPkt4 and PerfPkt6 with no additional
logic. Client classes would use PerfPkt4 and PerfPkt6 rather than
PktTransform so it makes sense in my opinion to focus on PerfPktX unit
testing. In the same time, adding tests for both would produce a
redundancy.
>
> '''pkt_transform.h'''
> writeValueAt() is intended to be used for integer types only, right?
Aren't there a simpler way to do it?
They are integers only. What is the simpler way?
>
> '''stats_mgr.h'''
> Why are two opertor++ definitions are needed?
I wanted to have ability to do either ++counter and counter++. In fact
just ++counter is the one used in the code but I don't see any problem
with having two operators just in case counter++ becomes needed some time.
>
> getValue(), getName() and similar - for simple methods no repetition of
\brief descriptions are needed, the brief description is sufficient.
I will do these changes in the course of #1960. This ticket will be mostly
to clean up the code.
>
> PktList type - I bow to your superior template handling techniques, sir.
How many nested templates are there? 6? :) Seriously speaking, I do not
understand that PktList definition.
I don't like having so many nested templates but in fact boost is about
templates and some of them may become quite complex in use. The
multi_index_container is documented here:
[http://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/index.html]
I also added some more comments in the typedef.
>
> sum_delay_squared_ field - Is this squared sum of delays or sum of
squared delays? Note that it is not the same. If it is the former, then it
is not needed and sum_delay_ may be used instead.
It is sum of squared delay values. I think I called that
squared_sum_delay_ initially but Stephen pointed out that it is wrong name
because it suggests that first we add non-squared values and then square
it. I renamed it then per Stephen's suggestion to sum_delay_squared_ which
is more appropriate. In this case you can see that there is no way to use
sum_delay_ instead.
>
> getDroppedPacketsNum(): Unless this method is expected to become more
complex in the future, the drops variable is not needed. You ca use:
> {{{
> if (getSentPacketsNum() > getRcvdPacketsNum()) {
> return (getSentPacketsNum() - getRcvdPacketsNum());
> } else {
> return (0);
> }
> }}}
>
That's right but I prefer having less indented code when possible. I have
many places like this in the code. I think there is too much overhead now
to change them all to the style you're proposing.
> printIntermediateStats(): ostringstream objects don't have to be
explicitly initialized to empty strings. The default constructor does that
as well, but is faster. ++it in "for" loop doesn't have to be written in a
separate line.
Corrected. Why are the default constructors faster here?
>
> Review finished on line 1502 of the patch. It is after 8pm Friday night.
The review will be continued on Monday.
Thank you for the useful comments.
--
Ticket URL: <http://bind10.isc.org/ticket/1959#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list