Fix potential data race in ESProductResolver #46330

wddgit · 2024-10-09T22:00:41Z

PR description:

Fixes a potential data race in ESProductResolver. Noticed while reading code for a different development. No one has reported an issue related to this. Probably it is not an actual issue because writing the same pointer value to the same memory location concurrently is not an actual problem on the CPUs we currently use, although technically it is a data race and might someday be a problem on CPUs in use in the future (we think...). This condition should also occur extremely rarely.

I removed the memory order specifications in this change because that is what we usually currently do, but if anyone thinks it worth it I will put them back. I just thought it was a remnant from when we used to use those in the early concurrency days.

Also removed an old comment about a global mutex that was removed a long time ago.

PR validation:

Existing unit tests pass. There shouldn't be any change in behavior or output.

cmsbuild · 2024-10-09T22:01:04Z

cms-bot internal usage

cmsbuild · 2024-10-09T22:04:27Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46330/42161

cmsbuild · 2024-10-09T22:04:53Z

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

It involves the following packages:

FWCore/Framework (core)

@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@makortel, @missirol 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

wddgit · 2024-10-09T22:07:16Z

please test

cmsbuild · 2024-10-10T04:59:59Z

+1

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

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 23 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3331066
DQMHistoTests: Total failures: 1694
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3329352
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

Dr15Jones · 2024-10-10T14:03:46Z

FWCore/Framework/src/ESProductResolver.cc

+        cache_.store(getAfterPrefetchImpl());
+        cache = cache_.load();


Suggested change

cache_.store(getAfterPrefetchImpl());

cache = cache_.load();

cache = cache_ = getAfterPrefetchImpl();

this may allow the compiler to generate more optimized code.

Done. Thanks, that is better.

cmsbuild · 2024-10-10T14:32:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46330/42171

wddgit · 2024-10-10T14:32:32Z

enable threading

cmsbuild · 2024-10-10T14:32:38Z

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

wddgit · 2024-10-10T14:32:40Z

please test

cmsbuild · 2024-10-10T19:07:32Z

-1

Failed Tests: RelVals RelVals-INPUT RelVals-THREADING
Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3fefca/42104/summary.html
COMMIT: 53d2f30
CMSSW: CMSSW_14_2_X_2024-10-09-2300/el8_amd64_gcc12
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46330/42104/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

4.53

----- Begin Fatal Exception 10-Oct-2024 19:48:18 CEST-----------------------
An exception of category 'LogicalFileNameNotFound' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing input source of type PoolSource
Exception Message:
RootFileSequenceBase::initTheFile()
Logical file name '++ echo /store/data/Run2012B/SinglePhoton/RAW/v1/000/194/533/1084D9DA-AEA2-E111-B3AB-001D09F24DA8.root' was not found in the file catalog.
If you wanted a local file, you forgot the 'file:' prefix
before the file name in your configuration file.
----- End Fatal Exception -------------------------------------------------

139.001

----- Begin Fatal Exception 10-Oct-2024 19:48:27 CEST-----------------------
An exception of category 'LogicalFileNameNotFound' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing input source of type PoolSource
Exception Message:
RootFileSequenceBase::initTheFile()
Logical file name '++ echo /store/data/Commissioning2021/MinimumBias/RAW/v1/000/346/512/00000/be4e0e99-6d25-4b6f-8648-1adefb79c7bf.root' was not found in the file catalog.
If you wanted a local file, you forgot the 'file:' prefix
before the file name in your configuration file.
----- End Fatal Exception -------------------------------------------------

136.731

----- Begin Fatal Exception 10-Oct-2024 19:48:27 CEST-----------------------
An exception of category 'LogicalFileNameNotFound' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing input source of type PoolSource
Exception Message:
RootFileSequenceBase::initTheFile()
Logical file name '++ echo /store/data/Run2016B/SinglePhoton/RAW/v2/000/274/199/00000/CA646D43-8526-E611-A6D9-02163E014300.root' was not found in the file catalog.
If you wanted a local file, you forgot the 'file:' prefix
before the file name in your configuration file.
----- End Fatal Exception -------------------------------------------------

Expand to see more relval errors ...

RelVals-INPUT

4.264.26_ZMuSkim2011A/step2_ZMuSkim2011A.log
4.174.17_RunMinBias2011A/step2_RunMinBias2011A.log
4.244.24_WMuSkim2011A/step2_WMuSkim2011A.log

Expand to see more relval errors ...

RelVals-THREADING

8.08.0_BeamHalo/step2_BeamHalo.log
4.534.53_RunPhoton2012B/step2_RunPhoton2012B.log
4.224.22_RunCosmics2011A/step2_RunCosmics2011A.log

Expand to see more relval errors ...

smuzaffar · 2024-10-10T19:09:54Z

please test

there was bug in cms-bot which is fix now

cmsbuild · 2024-10-11T00:08:58Z

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3fefca/42114/summary.html
COMMIT: 53d2f30
CMSSW: CMSSW_14_2_X_2024-10-10-1100/el8_amd64_gcc12
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46330/42114/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: 104 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3331066
DQMHistoTests: Total failures: 2917
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3328129
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

makortel · 2024-10-11T15:32:02Z

Comparison differences are related to #39803

makortel · 2024-10-11T15:32:27Z

+core

cmsbuild · 2024-10-11T15:32:44Z

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

makortel · 2024-10-11T15:39:53Z

1-7 % "increase" in maxmemory report is likely related to #46359

mandrenguyen · 2024-10-11T15:46:34Z

+1

cmsbuild added this to the CMSSW_14_2_X milestone Oct 9, 2024

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Oct 9, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 9, 2024

cmsbuild added tests-started and removed tests-pending labels Oct 9, 2024

cmsbuild added tests-approved and removed tests-started labels Oct 10, 2024

makortel mentioned this pull request Oct 10, 2024

Fix potential data race in ESProductResolver cms-sw/framework-team#1060

Closed

Dr15Jones reviewed Oct 10, 2024

View reviewed changes

Fix potential data race in ESProductResolver

53d2f30

wddgit force-pushed the potentialDataRaceESProductResolver branch from 123b6a1 to 53d2f30 Compare October 10, 2024 14:30

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Oct 10, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 10, 2024

cmsbuild added tests-started and removed tests-pending labels Oct 10, 2024

cmsbuild added tests-rejected and removed tests-started labels Oct 10, 2024

cmsbuild added tests-started and removed tests-rejected labels Oct 10, 2024

cmsbuild added tests-approved and removed tests-started labels Oct 11, 2024

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Oct 11, 2024

cmsbuild added orp-approved and removed orp-pending labels Oct 11, 2024

cmsbuild merged commit 4c50d04 into cms-sw:master Oct 11, 2024
12 checks passed

cmsbuild mentioned this pull request Oct 16, 2024

Update a few MC Gts in autoCond to the latest version available on 14/10/2024 #46379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential data race in ESProductResolver #46330

Fix potential data race in ESProductResolver #46330

wddgit commented Oct 9, 2024

cmsbuild commented Oct 9, 2024 •

edited

Loading

cmsbuild commented Oct 9, 2024

cmsbuild commented Oct 9, 2024

wddgit commented Oct 9, 2024

cmsbuild commented Oct 10, 2024

Dr15Jones Oct 10, 2024

wddgit Oct 10, 2024

cmsbuild commented Oct 10, 2024

wddgit commented Oct 10, 2024

cmsbuild commented Oct 10, 2024

wddgit commented Oct 10, 2024

cmsbuild commented Oct 10, 2024

smuzaffar commented Oct 10, 2024

cmsbuild commented Oct 11, 2024

makortel commented Oct 11, 2024

makortel commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

makortel commented Oct 11, 2024

mandrenguyen commented Oct 11, 2024

	cache_.store(getAfterPrefetchImpl());
	cache = cache_.load();
	cache = cache_ = getAfterPrefetchImpl();

Fix potential data race in ESProductResolver #46330

Fix potential data race in ESProductResolver #46330

Conversation

wddgit commented Oct 9, 2024

PR description:

PR validation:

cmsbuild commented Oct 9, 2024 • edited Loading

cmsbuild commented Oct 9, 2024

cmsbuild commented Oct 9, 2024

wddgit commented Oct 9, 2024

cmsbuild commented Oct 10, 2024

Comparison Summary

Dr15Jones Oct 10, 2024

Choose a reason for hiding this comment

wddgit Oct 10, 2024

Choose a reason for hiding this comment

cmsbuild commented Oct 10, 2024

wddgit commented Oct 10, 2024

cmsbuild commented Oct 10, 2024

wddgit commented Oct 10, 2024

cmsbuild commented Oct 10, 2024

RelVals

RelVals-INPUT

RelVals-THREADING

smuzaffar commented Oct 10, 2024

cmsbuild commented Oct 11, 2024

Comparison Summary

makortel commented Oct 11, 2024

makortel commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

makortel commented Oct 11, 2024

mandrenguyen commented Oct 11, 2024

cmsbuild commented Oct 9, 2024 •

edited

Loading