Opened 4 years ago

Closed 4 years ago

#4498 closed defect (fixed)

performance improvement: avoid copies of the container with option definitions in libdhcp++

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

Description

As a result of profiling DHCPv6 server I found that we're doing expensive copies of the container holding option definitions in all unpackOptions functions. That is:

  • LibDHCP::unpackOptions4
  • LibDHCP::unpackOptions6
  • Dhcpv6Srv::unpackOptions
  • Dhcpv6Srv::unpackOptions

The following code is a problem:

// Get the list of standard option definitions.
option_defs = LibDHCP::getOptionDefs(Option::V6);

It actually creates a copy of the container, which is very CPU intensive: ~15% of CPU time.

The following patch:

--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -2774,22 +2774,22 @@ Dhcpv6Srv::unpackOptions(const OptionBuffer& buf,
     size_t offset = 0;
     size_t length = buf.size();
 
-    OptionDefContainer option_defs;
+    const OptionDefContainer* option_defs = NULL;
     if (option_space == "dhcp6") {
         // Get the list of standard option definitions.
-        option_defs = LibDHCP::getOptionDefs(Option::V6);
+        option_defs = &LibDHCP::getOptionDefs(Option::V6);
     } else if (!option_space.empty()) {
         OptionDefContainerPtr option_defs_ptr =
             CfgMgr::instance().getCurrentCfg()->getCfgOptionDef()->
             getAll(option_space);
         if (option_defs_ptr != NULL) {
-            option_defs = *option_defs_ptr;
+            option_defs = option_defs_ptr.get();
         }
     }
 
     // Get the search index #1. It allows to search for option definitions
     // using option code.
-    const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+    const OptionDefContainerTypeIndex& idx = option_defs->get<1>();
 
     // The buffer being read comprises a set of options, each starting with
     // a two-byte type code and a two-byte length field.
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 700b734..9800be0 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -315,14 +315,14 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
     size_t last_offset = 0;
 
     // Get the list of standard option definitions.
-    OptionDefContainer option_defs;
+    const OptionDefContainer* option_defs = NULL;
     if (option_space == "dhcp6") {
-        option_defs = LibDHCP::getOptionDefs(Option::V6);
+        option_defs = &LibDHCP::getOptionDefs(Option::V6);
     } else {
         OptionDefContainerPtr option_defs_ptr =
             LibDHCP::getRuntimeOptionDefs(option_space);
         if (option_defs_ptr) {
-            option_defs = *option_defs_ptr;
+            option_defs = option_defs_ptr.get();
         }
     }
 
@@ -332,7 +332,8 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
 
     // Get the search index #1. It allows to search for option definitions
     // using option code.
-    const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
+
+    const OptionDefContainerTypeIndex& idx = option_defs->get<1>();
 

resulted in increase of performance by around ~1000 leases/sec, using memfile.

Subtickets

Change History (6)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per team meeting 12 May, accept 1.1. Estimate = .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

  • Add Hours to Ticket changed from 0 to 12
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 12

I went a bit more with the changes than updating existing "unpackOptions" methods. Because we currently register runtime option definitions in libdhcp++ there is no need to have server-side unpackOptions function, and we can simply rely on the LibDHCP::unpackOptions4 and LibDHCP::unpackOptions6. Because the purpose of this ticket was to improve performance of the unpackOptions I thought it is better to get rid of redundant copies of this function and apply fix in a single place, i.e. LibDHCP.

That also led to unification of types returned by functions like LibDHCP::getOptionDefs. Previously we had such methods return:

  • reference,
  • raw pointer,
  • smart pointer

Now, all such methods return smart pointer.

Because we don't use server side callback functions anymore, I thought we should remove them from the libdhcp++ API too. So, this may all look like many changes affecting many files. But, the changes are straightforward and shouldn't be difficult to review.

Proposed ChangeLog entry:

11XX.	[bug]		marcin
	libdhcp++: Removed unnecessary copies of the container holding
	option definitions to improve performance of both DHCPv4 and
	DHCPv6 server.
	(Trac #4498, git abcd)

comment:4 Changed 4 years ago by sar

  • Owner changed from UnAssigned to sar

comment:5 Changed 4 years ago by sar

  • Owner changed from sar to marcin

I've fixed a couple of typos and updated the copyrights on a couple of files, please pull before continuing

The changes look fine, I suggest updating the names for some tests as below, but other than that
it's ready for merging.

libdhcp++_unittest.cc

The test names for unpackSubOptions and unpackEmptyOption should have a suffix of 4 to be consistent.

comment:6 Changed 4 years ago by marcin

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

Merged with commit 14716853a92e08c4cc5be75ae85c5e84d6356a1e

Note: See TracTickets for help on using tickets.