Update deserialize function to return unique_ptr #25126
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25126/7123 |
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages: CondCore/CSCPlugins @tocheng, @cmsbuild, @franzoni, @ggovi, @pohsun, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test FYI @Dr15Jones |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
@wddgit |
|
+1 |
cmsbuild
added
db-approved
fully-signed
and removed
db-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
The effects of this pull request on code outside the file Serialization.h are small. Any code that fully specializes the deserialization function template needs to have its return type changed to unique_ptr. In addition, there is one file in CMSSW containing code like "auto x = deserialize ...", where on a subsequent line x is assigned to a shared_ptr. In this particular case, the variable could just be removed. It wasn't needed. An alternative in this case and other similar cases, would be the addition of std::move at the assignment. These problems are easy to find because they cause compilation to fail. All such cases in the CMSSW repository are fixed in this PR. There are not many. Calling deserialize directly is not common in user code, so I suspect there are not many problematic calls to deserialize outside the repository. In most cases I saw, code that uses deserialize will continue to work without any modification or performance problem.
Internally, the deserialize function does not save a copy of the shared_ptr. It does not actually manage the memory of the object. It is hard to imagine modifying the deserialize function in the future to manage this memory. That would imply the internals of deserialize would know when to delete the memory and also hold on to a copy of the shared_ptr.
Externally, we are making deserialize more flexible. Previously, the interface forced the client to use a shared_ptr or make an expensive deep copy. By returning a unique_ptr, the client has a choice. It can use a unique_ptr to manage the memory. But just as easily, it can convert to a shared_ptr. And because it is a move there is no performance penalty. (The inverse, converting from shared_ptr to unique_ptr does not work!)
In general, a unique_ptr performs better than a shared_ptr. With a shared_ptr there are two counters, we use CPU to increment/decrement them, and they use memory. If you do not use make_shared, there is an extra memory allocation. Also, the counters are thread safe and at run time must be synchronized across threads, which can be significant. I do not know if in this case the performance difference is significant.
Unique_ptr's are also easier to understand and think about for both developers and maintainers. See https://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/ for some discussion related to this in general.
The immediate motivation that started the core group looking at this relates to our goal of running multiple IOVs concurrently. Let's says an EDProducer processes two events and is using objects from the EventSetup from different IOVs. Assume that the values stored in the object for these different IOVs are different. When we ran one IOV at a time, one could have both objects actually be the same object at the same memory location. This allowed the ESProducer to implement some optimizations such as not having to reallocate the object at an IOV boundary or maybe only parts of the object would change at an IOV boundary. This worked fine when only one IOV is processed at a time. But when both IOVs are running simultaneously, this does not work. Unless you implement a very complex thread safe design for the object, it cannot hold two sets of values at the same time. The same object in the same memory cannot be used simultaneously with two different sets of values. In the past, the EventSetup implemented this reuse of objects by allowing the produce function to return a shared_ptr. The shared_ptr allows the ESProducer module to save a copy of the shared_ptr and reuse the object on the next call to produce. We now need to be careful that all ESProducers are operating in a thread safe manner. And it is important that we be able to verify this quickly. If we allocate new objects on each call to produce and return a unique_ptr, then we can verify this by simply looking at the return type of the produce function. But if a shared_ptr is returned we have to look inside the ESProducer at the details of how that ESProducer is using the shared memory internally and it gets complicated.
ESProducers can still return shared_ptr in some special cases and still work with concurrent IOVs, but in most cases it involves use of the new ReusableObjectHolder class which manages multiple copies of the object, one copy for each IOV. And even this will not work well if handed a shared_ptr that lower level code created.
If you look down the call stack from the produce function of many ESProducers you will find the deserialize function down near the bottom. There is no way to return a unique_ptr from these produce functions without changing the return type of the deserialize function (unless you do an expensive deep copy of the object or implement some horrible hack somewhere in between).
If this PR is approved, additional PRs will follow that modify the code higher in the stack above deserialize. However, this PR is not dependent on those changes. It could be approved alone. Even if none of the other changes are ever made, it would still be an improvement.
There is also another change included in this PR. There is a single one line change that resolves a clang warning. It is in a separate commit and unrelated.