[v6.24] Cling: prevent double release of Transactions #153

smuzaffar · 2021-03-31T18:21:25Z

No description provided.

new of a char array might not have the correct alignment to hold a Transaction. Allocate a Transaction itself directly, instead of in-place constructing it in the character array. Each Transaction in the pool is thus constructed through `new Transaction(S)` and destructed through `delete T`, which is nicely symmetrical. The use of `::operator new` and `::operator delete` isn't actually necessary. While I'm at it, improve the assert message's wording.

This will allow Transaction remembering its RAII to not depend on Interpreter.

This will allow the Intrepreter to prevent unload() on Transactions held by RAIIs.

When a Transaction is unloaded for which *also* a ScopedTransactionRAII is waiting, the latter will potentially access a deleted Transaction. It can be deleted due to the Pool being full. Even if it is not deleted (as is the case in root-project#7657 ) it will get pushed into the Pool once by unload() and a second time by the RAII! The two options to track this case were full-blown ref counting or noting that a Transaction is attached to an RAII scope. If that is the case, freeing the Transaction must be skipped, as the RAII will take care of it.

a cheap way to notice what went wrong in root-project#7657.

cmsbuild · 2021-03-31T18:21:42Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for branch cms/v6-24-00-patches/126c9c8.

@cmsbuild, @smuzaffar, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

smuzaffar · 2021-03-31T18:21:52Z

please test with cms-sw/cmsdist#6777

cmsbuild · 2021-04-01T01:21:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-feb1ca/13902/summary.html
COMMIT: 1847b1d
CMSSW: CMSSW_11_3_ROOT624_X_2021-03-29-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/root/153/13902/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: 84 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2640807
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2640778
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

smuzaffar · 2021-04-01T05:33:11Z

@slava77 , can you please check these reco comparison differences? These are for root 6.24 based cmssw

smuzaffar · 2021-04-01T06:09:20Z

please test with cms-sw/cmsdist#6777

smuzaffar · 2021-04-01T06:46:58Z

test parameters:

disable_poison = true

smuzaffar · 2021-04-01T06:47:35Z

please test with cms-sw/cmsdist#6777

slava77 · 2021-04-01T11:53:27Z

@slava77 , can you please check these reco comparison differences? These are for root 6.24 based cmssw

the differences look like the baseline was different in more than jut this PR; most likely in cms-sw/cmsdist#6769

the tests were done on top of CMSSW_11_3_ROOT624_X_2021-03-29-2300, which is made before cms-sw/cmsdist#6769 was merged.
It would be nice to add a summary of extra commits in the test summary.

smuzaffar · 2021-04-01T12:08:50Z

Thanks @slava77 for your analysis. As I have restarted the tests, so hopefully comparisons should be better now.
About the extra commits, yes looks like bot was only doing it when cmssw PRs are involved. I have updated it ( https://github.com/cms-sw/cms-bot/pull/1529/files ) and now extra commits should be added for all PR tests

cmsbuild · 2021-04-01T14:19:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-feb1ca/13915/summary.html
COMMIT: 1847b1d
CMSSW: CMSSW_11_3_ROOT624_X_2021-03-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/root/153/13915/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2640841
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2640812
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

smuzaffar · 2021-04-01T14:23:02Z

comparison results look better now

smuzaffar · 2021-04-01T16:46:35Z

please test with cms-sw/cms-bot#1530,cms-sw/cmsdist#6777

cmsbuild · 2021-04-02T06:38:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-feb1ca/13959/summary.html
COMMIT: 1847b1d
CMSSW: CMSSW_11_3_ROOT624_X_2021-03-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/root/153/13959/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:

[BuildRules] Fixes for multi-target releases cmsdist#6781 @cms-sw: [BuildRules] Fixes for multi-target releases
Update tag for GeneratorInterface-EvtGenInterface to V02-02-00 cmsdist#6780 @cms-sw: Update tag for GeneratorInterface-EvtGenInterface to V02-02-00

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

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

No significant changes to the logs found
Reco comparison results: 1260 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2640841
DQMHistoTests: Total failures: 3668
DQMHistoTests: Total nulls: 19
DQMHistoTests: Total successes: 2637132
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -45.703 KiB( 36 files compared)
DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

Axel-Naumann added 8 commits March 31, 2021 19:33

[cling] Move PushTransactionRAII out of Interpreter:

b9fd635

This will allow Transaction remembering its RAII to not depend on Interpreter.

[cling] Order includes alphabetically (NFC).

c229ee9

[cling] Transaction needs to know its RAII:

f71aae8

This will allow the Intrepreter to prevent unload() on Transactions held by RAIIs.

[cling] Assert that no nested attaches to unloading Transaction.

9328faf

[cling] Remove historical leftover in TransactionPool size (NFC).

574e88c

[cling] Assert released transaction is != last in pool:

1847b1d

a cheap way to notice what went wrong in root-project#7657.

cmsbuild added externals-pending orp-pending pending-signatures tests-pending labels Mar 31, 2021

cmsbuild added requires-external tests-started and removed tests-pending labels Mar 31, 2021

smuzaffar mentioned this pull request Mar 31, 2021

PyROOT in root 6.24 branch hangs while loading CMSSW library root-project/root#7718

Closed

cmsbuild added tests-approved and removed tests-started labels Apr 1, 2021

cmsbuild added tests-started and removed tests-approved labels Apr 1, 2021

smuzaffar mentioned this pull request Apr 1, 2021

always add extra merge commit even if there are no cmssw PRs involved cms-sw/cms-bot#1529

Closed

cmsbuild added tests-approved and removed tests-started labels Apr 1, 2021

cmsbuild added tests-started and removed tests-approved labels Apr 1, 2021

cmsbuild added tests-approved and removed tests-started labels Apr 2, 2021

smuzaffar closed this Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v6.24] Cling: prevent double release of Transactions #153

[v6.24] Cling: prevent double release of Transactions #153

smuzaffar commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

smuzaffar commented Mar 31, 2021

cmsbuild commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

slava77 commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

cmsbuild commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

cmsbuild commented Apr 2, 2021

[v6.24] Cling: prevent double release of Transactions #153

[v6.24] Cling: prevent double release of Transactions #153

Conversation

smuzaffar commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

smuzaffar commented Mar 31, 2021

cmsbuild commented Apr 1, 2021

Comparison Summary

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

slava77 commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

cmsbuild commented Apr 1, 2021

Comparison Summary

smuzaffar commented Apr 1, 2021

smuzaffar commented Apr 1, 2021

cmsbuild commented Apr 2, 2021

Comparison Summary