Opened 9 years ago

Closed 9 years ago

#537 closed task (complete)

Make asiolink::UDPServer smaller to copy

Reported by: vorner Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110209
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 6.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Currently 8% of runtime is spent on copying and destroying instances of UDPServer. While this is a problem by itself, solving this will be part of larger refactoring (it is a result of how the coroutines work). The intermediate solution to make the cost of copying less significant is by using the pimpl design pattern. If we put all important data into the private part that is not copied and have single shared pointer inside the UDPServer itself, the overhead should get significantly lower.

There are two variables (bytes_ and done_) that are not shared between instances right now, so care should be taken with them.

Also, some shared pointers inside the private part might be changed to auto_ptrs, as the private part should never be copied.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:2 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Done, in trac537. Branched from 7c42c6d38d63dd55462eb01037a55a40185920a3.

I measured improvements from 27307.577134QPS to 30685.542268.

comment:3 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 6

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:5 follow-up: Changed 9 years ago by jinmei

I don't see anything obviously wrong in the diff.

I'm not sure if we want to have this level of tuning at this point,
but since the changes are basically straightforward and not so big,
I don't oppose to that.

For a longer term I hope we'll completely eliminate all the overhead
of coroutine and per query resource allocation for b10-auth while
sharing as much code as possible with the resolver, but that's
certainly a separate topic, and not for now in any event.

Some other things I happened to notice:

  • (although this is in the original code) s/enternal/internal/?
    +    // The ASIO-enternal endpoint object representing the client
    
  • indentation style in multi-line statements was changed, e.g.
    -        (*answer_callback_)(*io_message_, query_message_,
    -                            answer_message_, respbuf_);
    +        (*data_->answer_callback_)(*data_->io_message_, data_->query_message_,
    +                            data_->answer_message_, data_->respbuf_);
    
    The original style is the (vaguely documented) convention in BIND 9 (see the "Indentation" section of http://bind10.isc.org/wiki/BIND9CodingGuidelines). As a fan of consistency I'd like to keep a consistent style (either way) if possible. But this is quite a minor point.

comment:6 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I'm not sure if we want to have this level of tuning at this point,
but since the changes are basically straightforward and not so big,
I don't oppose to that.

There's no time to do the proper tuning with redesigning and refactoring.

For a longer term I hope we'll completely eliminate all the overhead
of coroutine and per query resource allocation for b10-auth while
sharing as much code as possible with the resolver, but that's
certainly a separate topic, and not for now in any event.

Fully agreed.

Some other things I happened to notice:

  • (although this is in the original code) s/enternal/internal/?
    +    // The ASIO-enternal endpoint object representing the client
    

Hmm, I read it wrong the first time I saw it as „ethernal“ and wondered what it might mean. Fixed.

  • indentation style in multi-line statements was changed, e.g.
    -        (*answer_callback_)(*io_message_, query_message_,
    -                            answer_message_, respbuf_);
    +        (*data_->answer_callback_)(*data_->io_message_, data_->query_message_,
    +                            data_->answer_message_, data_->respbuf_);
    
    The original style is the (vaguely documented) convention in BIND 9 (see the "Indentation" section of http://bind10.isc.org/wiki/BIND9CodingGuidelines). As a fan of consistency I'd like to keep a consistent style (either way) if possible. But this is quite a minor point.

I didn't find it there anyway. But you're right this looked odd, I didn't update the indentation. I prefer indenting by 4 spaces as in any other nested statement, is that OK (as I changed it)? Stuff like this looks strange to me:

         really_long->function->call(one_param,
	                             second,
				     third ? or :
				     another);

comment:8 Changed 9 years ago by vorner

Oh, and it probably should have a changelog entry. What about this?

[refactor]
The pimpl design pattern is used in IOServer, with a shared pointer. This
makes it smaller to copy (which is done a lot as a sideeffect of being
coroutine) and speeds the server up by around 10%.

(I asked on the jabber room and we agreed this is not a feature nor a bug, and the refactor category was suggested.)

comment:9 in reply to: ↑ 7 Changed 9 years ago by jinmei

Replying to vorner:

I'm not sure if we want to have this level of tuning at this point,
but since the changes are basically straightforward and not so big,
I don't oppose to that.

There's no time to do the proper tuning with redesigning and refactoring.

Right, but my point is that there are certainly worse time to do
tuning that would introduce premature optimizations. We do not
necessarily have to wait for "the right time to do proper tuning" but
we should not do it in worse time.

For refactoring (for such purposes as better readability), I basically
agree it should be done as soon as we see possibility of it.

The original style is the (vaguely documented) convention in BIND 9
(see the "Indentation" section of
http://bind10.isc.org/wiki/BIND9CodingGuidelines). As a fan of
consistency I'd like to keep a consistent style (either way) if
possible. But this is quite a minor point.

I didn't find it there anyway. But you're right this looked odd, I didn't update the indentation. I prefer indenting by 4 spaces as in any other nested statement, is that OK (as I changed it)? Stuff like this looks strange to me:

         really_long->function->call(one_param,
	                             second,
				     third ? or :
				     another);

I personally prefer the "strange" style, because it better clarifies
the relationship between a function and its parameters. With the
"strange" style, we'll have

    function1(function2(function3(arg_for_function3_1, arg_for_function3_2),
                        arg_for_function2_1, arg_for_function2_2),
              arg_for_function1_1, arg_for_function1_2);

and we can easily understand arg_for_functionN_M are for functionN.

With the 4-space style, it would look:

    function1(function2(function3(arg_for_function3_1, arg_for_function3_2),
        arg_for_function2_1, arg_for_function2_2), arg_for_function1_1,
        arg_for_function1_2);

which is confusing to me. This is of course an artificial and extreme
example, but with C++ such deeply nested expression is not uncommon.

udpdns.cc has a more practical example:

        CORO_YIELD data_->socket_->async_send_to(
            buffer(data_->respbuf_->getData(), data_->respbuf_->getLength()),
            *data_->sender_, *this);

If this is indented in the 4-space style, I cannot be sure whether or
not "*data_->sender_" and "*this" are part of buffer() parameters. In
the "strange" style, it's obvious because otherwise it should look
like this:

        CORO_YIELD data_->socket_->async_send_to(
            buffer(data_->respbuf_->getData(), data_->respbuf_->getLength(),
                   *data_->sender_, *this));

Another reason why I prefer the "strange" style is that it's
consistent with BIND 9 (and apparently at least with ISC DHCP also).

But I also admit the "strange" style has disadvantage with longer
lines (this style tends to require more lines when function parameters
begin with a higher column), which is also quite common in C++.

Like any other style matters, this is largely a matter of preference
in the end, however, and I'm willing to adopt any agreed style. This
is probably a (bikeshed) topic to be discussed at bind10-dev?

In any case, I don't think this is a blocking issue of this ticket.
Feel free to move forward and merge.

comment:10 Changed 9 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed

Ok, thanks, merged.

Note: See TracTickets for help on using tickets.