Opened 5 years ago

Closed 5 years ago

#3715 closed defect (fixed)

Iface object spurious copies

Reported by: fdupont Owned by: marcin
Priority: low Milestone: Kea0.9.1
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Iface objects can be copied without necessity (cf #3712).

Subtickets

Change History (13)

comment:1 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.1
  • Priority changed from medium to low

comment:2 Changed 5 years ago by marcin

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

comment:3 Changed 5 years ago by marcin

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

This ticket is now ready for the review and here is a proposed changelog entry:

XXX.	[func]*		marcin
	libdhcp++: the C++ objects representing network interfaces
	(Iface objects) are now non-copyable for better performance.
	As a result, the API of the Interface Manager functions
	returning the pointers to the Iface objects has changed.
	(Trac #3715, git acbd)

I made the Iface class non-copyable to make sure that we don't do copies of this object anywhere. This class, besides the pre-allocated "receive buffer", contains some containers which are copied if the Iface is copied. In order to use the non-copyable Iface objects I changed the declaration of the IfaceCollection? container which now stores the pointers to the Iface objects, rather than objects themselves. Then, iterating over the containers holding pointers to the Iface objects makes it awkward to call functions of the Iface because we need to do something like:

    (*iface_it)->getName();

To make the code a little bit more readable I replaced the use of iterators with the BOOST_FOREACH construct. And, since I did this for the IfaceCollection stuff, I also changed this for other containers for consistency.

So, there is a lot of little changes in the code, but they are pretty straight forward.

The only issue is that the API of the libdhcp++ is now changing which may impact external applications that are based on the libdhcp++ (if any).

comment:4 Changed 5 years ago by fdupont

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

comment:5 Changed 5 years ago by fdupont

  • Status changed from accepted to reviewing

comment:6 follow-up: Changed 5 years ago by fdupont

IMHO the idea was not to remove all copies but to avoid unwanted copies, so I am against to make the class non-copyable and to use pointers. Instead I suggest to use references and a move for the push_back. And please keep us far from the template madness, so no BOOST stuff.

comment:7 Changed 5 years ago by fdupont

  • Owner changed from fdupont to marcin

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by marcin

  • Owner changed from marcin to UnAssigned

Replying to fdupont:

IMHO the idea was not to remove all copies but to avoid unwanted copies, so I am against to make the class non-copyable and to use pointers. Instead I suggest to use references and a move for the push_back. And please keep us far from the template madness, so no BOOST stuff.

If there is a way to not make copies, I am against making copies and I treat all as unwanted. We use boost in many places already, including boost foreach. Plus using iterators is actually using templates, isn't it?

The code is now clearer and I disagree with the whole comment of yours, and I push it back to review queue so as someone else presents his opinion. If it conforms to your opinion I am ok.

comment:9 Changed 5 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:10 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by tmark

  • Owner changed from tmark to marcin

Replying to marcin:

Replying to fdupont:

IMHO the idea was not to remove all copies but to avoid unwanted copies, so I am against to make the class non-copyable and to use pointers. Instead I suggest to use references and a move for the push_back. And please keep us far from the template madness, so no BOOST stuff.

If there is a way to not make copies, I am against making copies and I treat all as unwanted. We use boost in many places already, including boost foreach. Plus using iterators is actually using templates, isn't it?

The code is now clearer and I disagree with the whole comment of yours, and I push it back to review queue so as someone else presents his opinion. If it conforms to your opinion I am ok.

Making Iface non-copyable is appropriate not so much to avoid unnecessary copies but rather inadvertent copies. We do not want code accidentally working with more than one copy of the same interface. This might introduce weird data reads, writes, or socket states. From our "system" perspective there ought to be only one instance for each interface and making the class non-copyable the standard practice for ensuring this.

We cannot use C++/11 features and one cannot store references in STL containers (at least not without some sort wrapper code). So if not actual objects then it is either raw pointers or smart pointers. We strive not to use raw pointers unless necessary and containers of smart pointers are already widely used in Kea. They are pretty much the defacto standard approach within Kea. The fact that IfaceMgr? wasn't already doing so has more to do with it being one of the first classes implemented than anything else.

As to using BOOST, again we already use BOOST_FOREACH. Kea uses quite a few of the basic BOOST features. If we're going to start worrying about then we better take a hard look at our code base first. We cannot use C++/ll range loops (yet) and BOOST_FOREACH makes
for more readable code than manual loops.

As to changing the API, other than it maybe be mentioning it in ChangeLog, I do not see
this as a big concern. We are not yet at 1.0, and I doubt early adopters will fuss much if at all. Better now than after 1.0.

Templates are an integral part of C++ in the twenty-first century. If it's madness than we're doomed. Maybe we should rewrite it all in Java.

In a nutshell, the changes you made are consistent with Kea's code base and coding guidelines and have no issues with it.

Please merge your changes.

comment:11 in reply to: ↑ 10 Changed 5 years ago by fdupont

Making Iface non-copyable is appropriate not so much to avoid unnecessary copies but rather inadvertent copies. We do not want code accidentally working with more than one copy of the same interface. This might introduce weird data reads, writes, or socket states. From our "system" perspective there ought to be only one instance for each interface and making the class non-copyable the standard practice for ensuring this.

=> even I agree with your argument it doesn't mean the face must be non-copyable and unfortunately the last statement (making the class non-copyable the standard practice for ensuring this) is false (non-copyable is not enough).

We cannot use C++/11 features

=> std::move is available in C++/0x too.

one cannot store references in STL containers.

=> references are not C++ objects so of course you can't store them.

So if not actual objects then it is either raw pointers or smart pointers.

=> there is no problem to store the actual object if you don't copy it when it is stored (so the move).

As to using BOOST, again we already use BOOST_FOREACH.

=> IMHO you want BOOST_FOREACH because the right way is not yet available because it is >= C++/11 only (:-).

Templates are an integral part of C++ in the twenty-first century. If it's madness than we're doomed.

=> templates are madness (and I share this opinion with others, cf "Advantages and disadvantages" in the Template (C++) wikipedia".

Noe I said what I wanted to say. Please merge.

comment:12 Changed 5 years ago by marcin

It looks like a discussion about personal preferences for the programming languages and libraries, rather than anything else.

The design choices for Kea origin from the BIND10 where C++ and template libraries like STL and BOOST were chosen for the project. We should stick to design choices and not avoid using the libraries which are already used all over the place to create a fake impression for C++ "haters" that we're not using templates and other C++ specific features. If we really want to get away from using templates in Kea it means "start over the project"! As long as we carry on with this project, the comments about the "madness" related to templates are pointless.

comment:13 Changed 5 years ago by marcin

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

Merged with commit 7415c74e38e13385a75e7200cb23b7d6ca86df7f

Note: See TracTickets for help on using tickets.