Skip to content

[WIP] Use a lock to protect access to collection from TROOT::GetListOfCleanups#67

Closed
Dr15Jones wants to merge 1 commit intoroot-project:v6-02-00-patchesfrom
Dr15Jones:protectGetListOfCleanups-v6-02-00-patches
Closed

[WIP] Use a lock to protect access to collection from TROOT::GetListOfCleanups#67
Dr15Jones wants to merge 1 commit intoroot-project:v6-02-00-patchesfrom
Dr15Jones:protectGetListOfCleanups-v6-02-00-patches

Conversation

@Dr15Jones
Copy link
Copy Markdown
Collaborator

@Dr15Jones Dr15Jones commented Jun 15, 2015

The protection of the collection returned from TROOT::GetListOfCleanups()
was insufficient since thread related crashes could still occur. This
commit protects all uses except for those from the GUI.

@Dr15Jones
Use a lock to protect access to collection from TROOT::GetListOfClean…
57db09a 
…ups()

The protection of the collection returned from TROOT::GetListOfCleanups()
was insufficient since thread related crashes could still occur. This
commit protects all uses except for those from the GUI.
@Dr15Jones
Copy link
Copy Markdown
Collaborator Author

Dr15Jones commented Jun 15, 2015

@pcanal

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 15, 2015

unfortunately this is performance issue .. as this way of handling the problem basically serialize all destruction of object derived from TObject ... :( ... i.e. one need a performance read-write lock for a proper fix

@Dr15Jones
Copy link
Copy Markdown
Collaborator Author

Dr15Jones commented Jun 15, 2015

CMS is seeing a crash originating from this container.

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 15, 2015

I am not surprised, this is a know problem. If the performance is acceptable for the CMS case, please use the patch. For the general case, we need more work on it.

@Dr15Jones
Copy link
Copy Markdown
Collaborator Author

Dr15Jones commented Jun 15, 2015

How many TObjects get marked at 'kMustCleanup'? It is only under that condition that the lock will be taken in the destructor.

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 15, 2015

At the very least all objects associated with a TDirectory (including gROOT) and a TPad (most notably that mean histograms and trees).

@davidlt
Copy link
Copy Markdown
Contributor

davidlt commented Oct 13, 2015

ping^1

@vgvassilev
Copy link
Copy Markdown
Member

vgvassilev commented Jun 2, 2017

I guess this relates to #596. Maybe it'd be worth rebasing this PR.

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 2, 2017

Can one of the admins verify this patch?

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 2, 2017

This patch will be superseded by work subsequent to #596

@davidlt
Copy link
Copy Markdown
Contributor

davidlt commented Jun 2, 2017

@pcanal does it incl. cms-sw@308daba ?
This is a part we still apply at CMS.

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 2, 2017

Yes, this patch will (eventually) also be unnecessary (i.e. replaced by taking a read lock at this point that might be turned into a write lock as needed).

@vgvassilev
Copy link
Copy Markdown
Member

vgvassilev commented Jun 4, 2017

Ok, shall we close it then?

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 4, 2017

I will close this on another related PR when a proper replacement is uploaded. This will serve as a signal to the author to replace their temporary work-around.

@pcanal pcanal changed the title Use a lock to protect access to collection from TROOT::GetListOfCleanups [WIP] Use a lock to protect access to collection from TROOT::GetListOfCleanups Aug 24, 2017
@etejedor
Copy link
Copy Markdown
Contributor

etejedor commented Nov 27, 2017

Hi @pcanal, perhaps we can close this one already since it is superseded by the RWlock PRs you have been working on?

@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jan 11, 2018

This is superseed by adding a thread-safe mode to some ROOT collection and updating the global lock to have a Read-Write behavior (and update RecursiveRemove and friends accordingly).

@pcanal pcanal closed this Jan 11, 2018
@xvallspl xvallspl mentioned this pull request Mar 11, 2020
Closed
3 tasks
@ethereal-space-cadet16 ethereal-space-cadet16 mentioned this pull request May 31, 2022
Closed
guitargeek added a commit to guitargeek/root that referenced this pull request Mar 7, 2026
@guitargeek
[Python] Group RooFit pythonization tests in one file
9bbb310 
For small unit tests like the RooFit pythonization tests, the overhead
of starting the Python interpreter is dominant. That means the tests
should not be too granular. I think testing the RooFit pythonizations
with separate Python executions at the level of the different RooFit
classes was indeed too granular. Therefore, this commit suggests to have
a single RooFit pythonization unit test.

Running the unit tests before (single thread):
```txt
Test project /home/rembserj/code/root/root_build/bindings/pyroot/pythonizations/test
      Start 64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabscollection
 1/14 Test root-project#64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabscollection .......   Passed    1.09 sec
      Start 65: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabspdf-fitto
 2/14 Test root-project#65: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabspdf-fitto ........   Passed    1.13 sec
      Start 66: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabsreal-ploton
 3/14 Test root-project#66: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabsreal-ploton ......   Passed    1.14 sec
      Start 67: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooarglist
 4/14 Test root-project#67: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooarglist .............   Passed    0.93 sec
      Start 68: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roocmdarg
 5/14 Test root-project#68: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roocmdarg ..............   Passed    0.95 sec
      Start 69: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-numpy
 6/14 Test root-project#69: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-numpy ......   Passed    1.32 sec
      Start 70: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-ploton
 7/14 Test root-project#70: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-ploton .....   Passed    1.09 sec
      Start 71: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset
 8/14 Test root-project#71: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset .............   Passed    1.03 sec
      Start 72: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset-numpy
 9/14 Test root-project#72: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset-numpy .......   Passed    1.70 sec
      Start 73: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooglobalfunc
10/14 Test root-project#73: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooglobalfunc ..........   Passed    1.21 sec
      Start 74: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roojsonfactorywstool
11/14 Test root-project#74: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roojsonfactorywstool ...   Passed    1.40 sec
      Start 75: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roolinkedlist
12/14 Test root-project#75: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roolinkedlist ..........   Passed    0.87 sec
      Start 76: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roosimultaneous
13/14 Test root-project#76: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roosimultaneous ........   Passed    1.09 sec
      Start 77: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooworkspace
14/14 Test root-project#77: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooworkspace ...........   Passed    1.21 sec

100% tests passed, 0 tests failed out of 14

Label Time Summary:
python                 =  16.16 sec*proc (14 tests)
python_runtime_deps    =   3.02 sec*proc (2 tests)

Total Test time (real) =  16.17 sec
```

With this commit:
```txt
Test project /home/rembserj/code/root/root_build/bindings/pyroot/pythonizations/test
    Start 64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit
1/1 Test root-project#64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit ...   Passed    3.27 sec

100% tests passed, 0 tests failed out of 1

Label Time Summary:
python    =   3.27 sec*proc (1 test)

Total Test time (real) =   3.27 sec
```
guitargeek added a commit that referenced this pull request Mar 7, 2026
@guitargeek
[Python] Group RooFit pythonization tests in one file
2a74548 
For small unit tests like the RooFit pythonization tests, the overhead
of starting the Python interpreter is dominant. That means the tests
should not be too granular. I think testing the RooFit pythonizations
with separate Python executions at the level of the different RooFit
classes was indeed too granular. Therefore, this commit suggests to have
a single RooFit pythonization unit test.

Running the unit tests before (single thread):
```txt
Test project /home/rembserj/code/root/root_build/bindings/pyroot/pythonizations/test
      Start 64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabscollection
 1/14 Test #64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabscollection .......   Passed    1.09 sec
      Start 65: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabspdf-fitto
 2/14 Test #65: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabspdf-fitto ........   Passed    1.13 sec
      Start 66: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabsreal-ploton
 3/14 Test #66: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooabsreal-ploton ......   Passed    1.14 sec
      Start 67: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooarglist
 4/14 Test #67: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooarglist .............   Passed    0.93 sec
      Start 68: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roocmdarg
 5/14 Test #68: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roocmdarg ..............   Passed    0.95 sec
      Start 69: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-numpy
 6/14 Test #69: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-numpy ......   Passed    1.32 sec
      Start 70: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-ploton
 7/14 Test #70: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodatahist-ploton .....   Passed    1.09 sec
      Start 71: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset
 8/14 Test #71: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset .............   Passed    1.03 sec
      Start 72: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset-numpy
 9/14 Test #72: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roodataset-numpy .......   Passed    1.70 sec
      Start 73: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooglobalfunc
10/14 Test #73: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooglobalfunc ..........   Passed    1.21 sec
      Start 74: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roojsonfactorywstool
11/14 Test #74: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roojsonfactorywstool ...   Passed    1.40 sec
      Start 75: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roolinkedlist
12/14 Test #75: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roolinkedlist ..........   Passed    0.87 sec
      Start 76: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roosimultaneous
13/14 Test #76: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-roosimultaneous ........   Passed    1.09 sec
      Start 77: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooworkspace
14/14 Test #77: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit-rooworkspace ...........   Passed    1.21 sec

100% tests passed, 0 tests failed out of 14

Label Time Summary:
python                 =  16.16 sec*proc (14 tests)
python_runtime_deps    =   3.02 sec*proc (2 tests)

Total Test time (real) =  16.17 sec
```

With this commit:
```txt
Test project /home/rembserj/code/root/root_build/bindings/pyroot/pythonizations/test
    Start 64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit
1/1 Test #64: pyunittests-bindings-pyroot-pythonizations-pyroot-roofit ...   Passed    3.27 sec

100% tests passed, 0 tests failed out of 1

Label Time Summary:
python    =   3.27 sec*proc (1 test)

Total Test time (real) =   3.27 sec
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@pcanal pcanal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Dr15Jones @pcanal @davidlt @vgvassilev @phsft-bot @etejedor