[Python] Set memory policy to "strict"#230
Closed
smuzaffar wants to merge 91 commits intocms-sw:cms/master/a0fb33955e5from
Closed
[Python] Set memory policy to "strict"#230smuzaffar wants to merge 91 commits intocms-sw:cms/master/a0fb33955e5from
smuzaffar wants to merge 91 commits intocms-sw:cms/master/a0fb33955e5from
Conversation
Idea to have interface which handle painting of many segments at once. On platforms like X11 drawing of many lines at once can be performed with single call
Apply clip condition, draw on virtualx and virtualps
Here no clipping is required, for VirtualPS coordinates need to be recalculated
Make lambda to paint ticks at specified index/position So reduce code duplication when painting extra ticks on left and right sides. Prepare code to fill buffers for segments painting
First collect all ticks and grids positions in the vector and then call function at the end
In SVG files all grids lines drawn together after all ticks so order is changed. But produced image will not change while grids always drawn on the top of the ticks PS image can reduce in size while now attributes not switched between ticks and grids all the time
Provide default implementation to ensure that all possible derived classes will work with new code
So first recalculate/clip buffers and then call methods
Provide default implementation which makes simple drawing of N individual lines. While method `DrawSegments` with low-level API is already exist use name `DrawLinesSegments`
Like with TGX11::DrawPolyLine, split drawing on smaller portions if more than 500000 segments provide at a time. Use direct cast from `TPoint *` to `XSegment *` while segment is just two points one after another
Now native graphics can use faster X11 routine
It should allow to optimize drawing of the segments on SVG/PDF outputs
Recalculate buffer when using NDC - while PS always used only user coordinates
Many segments of similar line style combined now together in same svg:path element
stressGraphics.ref changed while dummy segments (zero-size length) not drawn at all. So ticks of zero size not appear in PS and PDF files as well. In svg_ref now axis ticks drawn as segments so now is the only svg:path for all axis ticks are placed. Main content of SVG files did not change so it is minimal amount of data commited into repository
While labels and grid/ticks painting was mixed, move produced labels in intermediate buffer to paint them between ticks and grids
After change in TGaxis segments for log scale are used. This makes more compact SVG and also PS can be shorter while all ticks drawn before all grids
This follows up on 4ae08b3, fixing the Windows builds where this C++20 feature is not available.
If an interface is meant to be used for passing ownership, this should be implemented with an explicit Pythonization instead of relying on heuristics in cppyy, because the heuristics result in many false positives for ownership transfer, manifesting as memory leaks. We would like to avoid using the heuristic memory policy in the future, and this change is done in preparation for that. This commit has absolutely no effect on the behavor of the ROOT Python interface at this point, because it's redundant with the heuristic memory policy. But these Pythonization will be important once the strict memory policy is the default.
Remove the `builtin_glew` dependency, so one always requires having glew on the system for now, until we migrate from glew to GLAD. No deprecation or erroring out on that option is required, because it's simply ignored now: glew will never be required anymore anyway as of the next commits that will migrate from GLEW to GLAD.
Add minimal OpenGL loader code generated by the Glad tool: https://github.com/Dav1dde/glad Co-authored-by: Matevz Tadel <matevz.tadel@cern.ch>
In particular, this includes Wrapper functions - written by @bellenot - that fix an issue with several glad functions not compatible with `gluTessCallback`. This is visible with the tutorial grad.C, not rendering the color gradients if these wrapper functions are not used. Co-authored-by: Bertrand Bellenot <Bertrand.Bellenot@cern.ch>
Closes root-project#18471. Co-authored-by: Bertrand Bellenot <Bertrand.Bellenot@cern.ch> Co-authored-by: Matevz Tadel <matevz.tadel@cern.ch>
The `RModel::AddOperatorReference()` function is not memory safe, because it takes ownership of an argument passed by raw pointer. It's better to remove these kind of functions (possible because SOFIE is still in the experimental namespace), and use the safe alternative that take a `std::unique_ptr` instead, for clear ownership semantics.
It's better to centralize this logic to make sure there is no confusion about what these data member names are.
guitargeek
force-pushed
the
pyroot_memory_policy_1
branch
from
e787a88 to
8a8e854
Compare
|
Pull request #230 was updated. |
Author
|
please test with cms-sw/cmsdist#10395 using full cmssw for CMSSW_16_1_ROOT6_X |
Author
|
please test with cms-sw/cmsdist#10395 using full cmssw for CMSSW_16_1_ROOT6_X |
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
```
This follows up on 7ac08ad, ensuring the object ownership is also handled correctly in `TH2Poly::AddBin()`. This is motivated by usage of `TH2Poly` in CMSSW unit tests, and a ROOT unit test that covers the same usage pattern is now added as well.
Clear the TList at the end of the `test_delitem` unit test in tseqcollection_itemaccess.py to make sure the contained list elements are still alive when the list is cleared. Otherwise, the elements might be deleted before the containing list, depending on the order or garbage collection.
The ROOT Python interfaces have many memory leaks, which is a major pain point for people using it for long-running scripts in batch jobs. One source of memory leaks was indentified to be the "heuristic memory policy" of cppyy. This means that cppyy assumes that every non-const pointer member function argument was interpreted as the object taking ownership if the argument. For examle, take the non-owning RooLinkedList container. It has a `RooLinkedList::Add(RooAbsArg *arg)` method. ROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the ROOT overship. Nobody feels responsible for deleting the object anymore, and there is a memory leak or `arg`. That particular leak was reported in this forum post: https://root-forum.cern.ch/t/memory-leak-in-fits/56249 Function parameters of type `T *` are very common in ROOT, and only rarely do they imply ownership transfer. So changing the memory policy to "strict" would surely fix also many other memory leaks that are not reported so far. In fact, upstream cppyy doesn't even enable this heuristic memory policy anymore by default! So moving ROOT also to the strict memory policy closes the gap between ROOT and cppyy. The potential drawback of this change are crashes in usercode if memory is not properly managed. But these problems should either be fixed by: * the user * dedicated pythonizations for these methods to manage shared ownership via Python reference counters (i.e., setting the parameter as an attribute of the object that the member function was called on) This follows up on PR root-project#4294, in particular it reverts 3a12063. Note that owning **TCollection**, **TSeqCollection**, and **TList** instances are Pythonized to preserve the old behavior of dropping Python ownership of added elements, as that was the case where the memory heuristic was correct.
guitargeek
force-pushed
the
pyroot_memory_policy_1
branch
from
8a8e854 to
4162265
Compare
|
Pull request #230 was updated. |
Author
|
please test using full cmssw for CMSSW_16_1_ROOT6_X |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2dc4c7/51849/summary.html Comparison SummarySummary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
15 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CMSSW tests for root-project#13593