Simplify the ReusableObjectHolder #36068

fwyzard · 2021-11-10T07:09:54Z

PR description:

Simplify the ReusableObjectHolder:

remove unused includes
reduce code duplication
do not add new objects to the cache before returning them
check if try_pop was successful instead of the value of the temporary objects

PR validation:

Unit tests pass.

…rning them

…e of the object

fwyzard · 2021-11-10T07:10:02Z

please test

cmsbuild · 2021-11-10T07:17:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36068/26537

This PR adds an extra 20KB to repository

cmsbuild · 2021-11-10T07:17:29Z

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

FWCore/Utilities (core)

@makortel, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2021-11-10T10:41:39Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-197053/20423/summary.html
COMMIT: ca0e6ab
CMSSW: CMSSW_12_2_X_2021-11-09-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36068/20423/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901868
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

fwyzard · 2021-11-10T23:04:56Z

By the way, @Dr15Jones @makortel, I think the example for the makeOrGetAndClear method is misleading.
It says:

The primary way of using the class it to call makeOrGetAndClear
An example use would be

auto objectToUse = holder.makeOrGetAndClear(
                            []() { return new MyObject(10); }, //makes new one
                            [](MyObject* old) {old->reset(); } //resets old one
                  );

which seems to imply that old->reset() is called only for objects that are reused.
However, looking at the original implementation:

template< typename FM, typename FC>
std::shared_ptr<T> makeOrGetAndClear( FM iMakeFunc, FC iClearFunc) {
        std::shared_ptr<T> returnValue;
        while ( ! ( returnValue = tryToGet()) ) {
                add( std::unique_ptr<T>(iMakeFunc()) );
        }
        iClearFunc(returnValue.get());
        return returnValue;
}

I think that iClearFunc() is called for any objects before it is returned.

If you confirm that this is desired behaviour (hopefully simplified but unchanged by this PR) I can update the comments as well.

Dr15Jones · 2021-11-11T16:41:19Z

FWCore/Utilities/interface/ReusableObjectHolder.h

-      while (!(returnValue = tryToGet())) {
-        add(makeUnique(iMakeFunc()));
-      }
+      std::shared_ptr<T> returnValue = makeOrGet(iMakeFunc);


I believe doing

Suggested change

std::shared_ptr<T> returnValue = makeOrGet(iMakeFunc);

std::shared_ptr<T> returnValue = makeOrGet(std::move(iMakeFunc));

would avoid a possible copy of the functor.

Possibly an even better solution would be to change the function signature to

makeOrGetAndClear(FM&& iMakeFunc, FC&& iClearFunc)

and then do

Suggested change

std::shared_ptr<T> returnValue = makeOrGet(iMakeFunc);

std::shared_ptr<T> returnValue = makeOrGet(std::forward<FM>(iMakeFunc));

A similar function signature change to makeOrGet would probably be required to get the most benefit of such a change.

Good catch, thanks.

I think this change

template <typename FM, typename FC> std::shared_ptr<T> makeOrGetAndClear(FM&& iMakeFunc, FC iClearFunc) { std::shared_ptr<T> returnValue = makeOrGet(std::forward<FM>(iMakeFunc)); iClearFunc(returnValue.get()); return returnValue; }

should be enough, since iMakeFunc is the only one that is copied.

At least, my confused understanding is that FC iClearFunc (in makeOrGetAndClear) and F iFunc (in makeOrGet) should already take any of the functors by reference if they are lvalues.

I think using FC instead of FC&& means the compiler (before optimization) will call the move constructor of FC when passed an lvalue. I'd bet that all compilers will remove that call when optimation is turned on. However, using FC&& does make the requirements of the arguments more explicit.

I was wrong. I tested in godbolt and for the case where the object is created directly in the argument list they are identical.

It does appear that if the argument is created outside of the function argument list, then FC&& is the better choice as it does avoid an extra copy.

https://godbolt.org/z/7fYG641qq

OK, done - let me know if I missed anything (here or elsewhere).

Sorry, I missed the last comment...

...done, now.

fwyzard · 2021-11-11T17:20:49Z

please test

cmsbuild · 2021-11-11T17:25:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36068/26575

This PR adds an extra 16KB to repository

fwyzard · 2021-11-12T06:32:54Z

If you confirm that this is desired behaviour (hopefully simplified but unchanged by this PR) I can update the comments as well.

The unit test seem to agree that iClearFunc should always be called, both on reused and on new objects, e.g.:

cmssw/FWCore/Utilities/test/reusableobjectholder_t.cppunit.cpp

Lines 67 to 75 in c272d72

    
           { 
        
             edm::ReusableObjectHolder<int> intHolder; 
        
             auto p = intHolder.makeOrGetAndClear([]() -> int* { return new int(1); }, [](int* iV) { *iV = 0; }); 
        
             CPPUNIT_ASSERT(p.get() != 0); 
        
             CPPUNIT_ASSERT(*p == 0); 
        
             auto p2 = intHolder.tryToGet(); 
        
             CPPUNIT_ASSERT(p2.get() == 0); 
        
           }

So, I'm not sure how

   auto p = intHolder.makeOrGetAndClear([]() -> int* { return new int(1); }, [](int* iV) { *iV = 0; });

is better than

   auto p = intHolder.makeOrGet([]() -> int* { return new int(1); }); 
   *p = 0;

?

It doesn't seem to be used anywhere in CMSSW. On the other hand, it doesn't add any complexity or cost.

I'll update the comments, though.

fwyzard · 2021-11-12T06:49:51Z

please test

cmsbuild · 2021-11-12T06:54:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36068/26592

This PR adds an extra 20KB to repository

cmsbuild · 2021-11-12T06:55:00Z

Pull request #36068 was updated. @makortel, @smuzaffar, @Dr15Jones can you please check and sign again.

cmsbuild · 2021-11-12T10:21:06Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-197053/20486/summary.html
COMMIT: 35efc5c
CMSSW: CMSSW_12_2_X_2021-11-11-2300/slc7_amd64_gcc900
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36068/20486/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2902040
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2902006
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

makortel · 2021-11-12T20:15:17Z

+1

cmsbuild · 2021-11-12T20:15:41Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-11-12T22:40:02Z

+1

fwyzard added 4 commits November 10, 2021 07:28

ReusableObjectHolder: remove unused header

7d5d8dd

ReusableObjectHolder: reduce code duplication

a902d15

ReusableObjectHolder: do not add new objects to the cache before retu…

28cd464

…rning them

ReusableObjectHolder: check the result of try_pop instead of the valu…

ca0e6ab

…e of the object

cmsbuild added this to the CMSSW_12_2_X milestone Nov 10, 2021

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-started labels Nov 10, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 10, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 10, 2021

Dr15Jones reviewed Nov 11, 2021

View reviewed changes

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Nov 11, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 11, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 11, 2021

cmsbuild added the tests-approved label Nov 12, 2021

ReusableObjectHolder: update class documentation

35efc5c

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Nov 12, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 12, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 12, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 12, 2021

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Nov 12, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 12, 2021

cmsbuild merged commit dfc1d32 into cms-sw:master Nov 12, 2021

This was referenced Nov 13, 2021

Include EndLuminosityBlock transition in the esConsumes for LumiAlCaRecoProducer #36114

Merged

rootmodule: Updated root to tip of branch master cms-sw/cmsdist#7449

Merged

fwyzard deleted the CMSSW_12_2_X_simplify_ReusableObjectHolder branch July 31, 2022 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the ReusableObjectHolder #36068

Simplify the ReusableObjectHolder #36068

fwyzard commented Nov 10, 2021

fwyzard commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

fwyzard commented Nov 10, 2021

Dr15Jones Nov 11, 2021

fwyzard Nov 11, 2021

fwyzard Nov 11, 2021

fwyzard Nov 11, 2021 •

edited

Loading

Dr15Jones Nov 11, 2021

Dr15Jones Nov 11, 2021

Dr15Jones Nov 11, 2021

fwyzard Nov 11, 2021

fwyzard Nov 11, 2021

fwyzard Nov 11, 2021

fwyzard commented Nov 11, 2021

cmsbuild commented Nov 11, 2021

fwyzard commented Nov 12, 2021

fwyzard commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

makortel commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

perrotta commented Nov 12, 2021

	std::shared_ptr<T> returnValue = makeOrGet(iMakeFunc);
	std::shared_ptr<T> returnValue = makeOrGet(std::move(iMakeFunc));

Simplify the ReusableObjectHolder #36068

Simplify the ReusableObjectHolder #36068

Conversation

fwyzard commented Nov 10, 2021

PR description:

PR validation:

fwyzard commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

Comparison Summary

fwyzard commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Nov 11, 2021

cmsbuild commented Nov 11, 2021

fwyzard commented Nov 12, 2021

fwyzard commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

Comparison Summary

makortel commented Nov 12, 2021

cmsbuild commented Nov 12, 2021

perrotta commented Nov 12, 2021

fwyzard Nov 11, 2021 •

edited

Loading