Opened 4 years ago

Closed 4 years ago

#4495 closed defect (fixed)

Inefficient DHCP type option instance creation causes performance degradation

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.1
Component: libdhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 1 Internal?: no


The following code within the Pkt4::setType appears to be highly inefficient:

        // There is no message type option yet, add it
        std::vector<uint8_t> tmp(1, dhcp_type);
        opt = OptionPtr(new OptionInt<uint8_t>(Option::V4, DHO_DHCP_MESSAGE_TYPE,
                                               tmp.begin(), tmp.end()));

as it creates an instance of the DHCP type option using the constructor which parses the wire data. Because the setType() option is called fairly common this consumes a lot of CPU bandwidth (which is also visible on the profiling tests I have done).

The proper way to create an option instance would be:

        opt.reset(new OptionInt<uint8_t>(Option::V4, DHO_DHCP_MESSAGE_TYPE, dhcp_type));

This may result in server performance increase by hundreds of leases per second, as I observed running some internal tests with perfdhcp.


Change History (5)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

per 5/5 team meeting, accept 1.1 .25d

comment:2 Changed 4 years ago by marcin

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

comment:3 Changed 4 years ago by marcin

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

I have made this simple change.

Proposed ChangeLog entry:

11XX.	[bug]		marcin
	Performance improvement in libdhcp++: improved efficiency of the
	DHCPv4 Message Type option creation.
	(Trac #4495, git abcd)

comment:4 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to marcin
  • Total Hours changed from 0 to 1

There are 2 points: change the constructor and replace the assignment by a reset. Both are correct and faster so the code is IMHO OK. These changes are supposed to be indirectly user visible so I agree about the ChangeLog entry.

comment:5 Changed 4 years ago by marcin

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

Merged with commit 41c43a2a9e34931fc3ebf58c459f10ad08575d19

Note: See TracTickets for help on using tickets.