Avoid allocations in ParentageID #46729

Dr15Jones · 2024-11-18T21:33:45Z

PR description:

We were cycling memory each time a module put a data product into the event. This now has the ParentageID hold the 16 bytes directly instead of indirectly via a std::string.

PR validation:

All framework unit tests pass.

We were cycling memory each time a module put a data product into the event. This now has the ParentageID hold the 16 bytes directly instead of indirectly via a std::string.

cmsbuild · 2024-11-18T21:34:10Z

cms-bot internal usage

Dr15Jones · 2024-11-18T21:34:31Z

please test

cmsbuild · 2024-11-18T21:36:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46729/42699

cmsbuild · 2024-11-18T21:36:58Z

A new Pull Request was created by @Dr15Jones for master.

It involves the following packages:

DataFormats/Provenance (core)

@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@makortel, @missirol, @mmusich, @rovere, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2024-11-19T03:59:48Z

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e78c2f/42939/summary.html
COMMIT: 159a737
CMSSW: CMSSW_14_2_X_2024-11-18-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46729/42939/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e78c2f/42939/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e78c2f/42939/git-merge-result

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3344340
DQMHistoTests: Total failures: 469
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3343851
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 202 log files, 172 edm output root files, 46 DQM output files
TriggerResults: no differences found

Dr15Jones · 2024-11-19T14:09:14Z

One question is should

cmssw/DataFormats/Provenance/src/ProductProvenanceLookup.cc

Lines 42 to 57 in f472813

    
           void ProductProvenanceLookup::insertIntoSet(ProductProvenance entryInfo) const { 
        
             //NOTE:do not read provenance here because we only need the full 
        
             // provenance when someone tries to access it not when doing the insert 
        
             // doing the delay saves 20% of time when doing an analysis job 
        
             //readProvenance(); 
        
             auto itFound = 
        
                 std::lower_bound(entryInfoSet_.begin(), 
        
                                  entryInfoSet_.end(), 
        
                                  entryInfo.branchID(), 
        
                                  [](auto const& iEntry, edm::BranchID const& iValue) { return iEntry.branchID() < iValue; }); 
        
             if UNLIKELY (itFound == entryInfoSet_.end() or itFound->branchID() != entryInfo.branchID()) { 
        
               throw edm::Exception(edm::errors::LogicError) << "ProductProvenanceLookup::insertIntoSet passed a BranchID " 
        
                                                             << entryInfo.branchID().id() << " that has not been pre-registered"; 
        
             } 
        
             itFound->threadsafe_set(entryInfo.moveParentageID()); 
        
           }

be changed to deal with the fact that we do not need to do a move anymore?

makortel · 2024-11-19T14:52:07Z

One question is should ... be changed to deal with the fact that we do not need to do a move anymore?

We agreed that it should be changed.

makortel · 2024-11-19T15:00:04Z

The full build log shows

>> Building shared library tmp/el8_amd64_gcc12/src/DataFormats/Provenance/src/DataFormatsProvenance/libDataFormatsProvenance.so
...
In member function 'deallocate',
    inlined from 'deallocate' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/allocator.h:200:35,
    inlined from 'deallocate' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/alloc_traits.h:496:23,
    inlined from '_M_destroy' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/basic_string.h:300:34,
    inlined from '_M_dispose' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/basic_string.h:294:14,
    inlined from '_M_dispose' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/basic_string.h:291:7,
    inlined from '__dt_base ' at /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/basic_string.h:803:19,
    inlined from 'compactForm_' at src/DataFormats/Provenance/src/Hash.cc:25:5:
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02864/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/bits/new_allocator.h:158:33: warning: 'operator delete' called on unallocated object 'temp' [-Wfree-nonheap-object]
  158 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
src/DataFormats/Provenance/src/Hash.cc: In function 'compactForm_':
src/DataFormats/Provenance/src/Hash.cc:22:18: note: declared here
   22 |       value_type temp(hash);
      |                  ^

which seems to be coming from LTO, but from code not touched by this PR. With @Dr15Jones we have no clue what could be causing the warning.

@smuzaffar Is this warning not being flagged in the PR test summary because the Hash.cc was not touched by this PR?

I also noticed this warning was shown a second time later in the build log for the same shared library (including the same Building shared library line). @smuzaffar Do you know why that happens?

Dr15Jones · 2024-11-19T15:13:05Z

please test

smuzaffar · 2024-11-19T15:13:08Z

@makortel , we only report warnings which comes directory from cmssw files e.g. warnings which match cmssw/file/path:[0-9]+:[0-9]+: warning: .... . As this warning is coming from compiler header that is why bot is not going to report it.
Also warning coming from cmssw files which are not directly or indirectly touched by PR are not reported by bot

cmsbuild · 2024-11-22T14:52:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46729/42744

cmsbuild · 2024-11-22T14:52:49Z

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

Dr15Jones · 2024-11-22T14:56:25Z

please test

cmsbuild · 2024-11-22T19:55:19Z

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e78c2f/43011/summary.html
COMMIT: 4fb8f1a
CMSSW: CMSSW_14_2_X_2024-11-22-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46729/43011/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 2 lines from the logs
Reco comparison results: 14 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3483722
DQMHistoTests: Total failures: 525
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3483177
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 202 log files, 172 edm output root files, 46 DQM output files
TriggerResults: found differences in 1 / 44 workflows

makortel · 2024-11-22T21:17:40Z

Comparison differences are related to #46416

makortel · 2024-11-22T21:17:43Z

+core

cmsbuild · 2024-11-22T21:18:04Z

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. @mandrenguyen, @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

makortel · 2024-11-22T21:19:06Z

Number of allocations in workflow 12834.0 step 3 decreased by 72115, or ~7200/event.

In the latest test the number of allocations in the same job decreases by 73131

mandrenguyen · 2024-11-24T09:33:34Z

+1

Avoid allocations in ParentageID

159a737

We were cycling memory each time a module put a data product into the event. This now has the ParentageID hold the 16 bytes directly instead of indirectly via a std::string.

cmsbuild added this to the CMSSW_14_2_X milestone Nov 18, 2024

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending changes-dataformats labels Nov 18, 2024

cmsbuild added tests-started and removed tests-pending labels Nov 18, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 18, 2024

makortel mentioned this pull request Nov 18, 2024

Avoid allocations in ParentageID cms-sw/framework-team#1093

Closed

cmsbuild added tests-approved and removed tests-started labels Nov 19, 2024

moving ParentageID no longer gives any advantage

2d522c3

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Nov 19, 2024

cmsbuild modified the milestones: CMSSW_14_2_X, CMSSW_15_0_X Nov 22, 2024

Digest::fromHexifiedString now uses a std::string_view

4fb8f1a

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Nov 22, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 22, 2024

cmsbuild added tests-started and removed tests-pending labels Nov 22, 2024

cmsbuild added tests-approved and removed tests-started labels Nov 22, 2024

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Nov 22, 2024

cmsbuild added orp-approved and removed orp-pending labels Nov 24, 2024

cmsbuild merged commit 6bd6c74 into cms-sw:master Nov 24, 2024
11 checks passed

Dr15Jones deleted the noAllocParentageID branch November 25, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid allocations in ParentageID #46729

Avoid allocations in ParentageID #46729

Dr15Jones commented Nov 18, 2024

cmsbuild commented Nov 18, 2024 •

edited

Loading

Dr15Jones commented Nov 18, 2024

cmsbuild commented Nov 18, 2024

cmsbuild commented Nov 18, 2024

cmsbuild commented Nov 19, 2024

Dr15Jones commented Nov 19, 2024

makortel commented Nov 19, 2024

makortel commented Nov 19, 2024

Dr15Jones commented Nov 19, 2024

smuzaffar commented Nov 19, 2024

cmsbuild commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

Dr15Jones commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

makortel commented Nov 22, 2024

makortel commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

makortel commented Nov 22, 2024

mandrenguyen commented Nov 24, 2024

Avoid allocations in ParentageID #46729

Avoid allocations in ParentageID #46729

Conversation

Dr15Jones commented Nov 18, 2024

PR description:

PR validation:

cmsbuild commented Nov 18, 2024 • edited Loading

Dr15Jones commented Nov 18, 2024

cmsbuild commented Nov 18, 2024

cmsbuild commented Nov 18, 2024

cmsbuild commented Nov 19, 2024

Comparison Summary

Dr15Jones commented Nov 19, 2024

makortel commented Nov 19, 2024

makortel commented Nov 19, 2024

Dr15Jones commented Nov 19, 2024

smuzaffar commented Nov 19, 2024

cmsbuild commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

Dr15Jones commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

Comparison Summary

makortel commented Nov 22, 2024

makortel commented Nov 22, 2024

cmsbuild commented Nov 22, 2024

makortel commented Nov 22, 2024

mandrenguyen commented Nov 24, 2024

cmsbuild commented Nov 18, 2024 •

edited

Loading