Simplify memory regulation logic#16837
Merged
vepadulano merged 3 commits intoroot-project:masterfrom Nov 8, 2024
Merged
Conversation
vepadulano
changed the title
vepadulano
changed the title
vepadulano
force-pushed
the
pyroot-tmemoryregulator-destruction-order
branch
from
adf6a99 to
b8589d7
Compare
Test Results 19 files 19 suites 4d 9h 35m 18s ⏱️ Results for commit 4baf51d. ♻️ This comment has been updated with latest results. |
vepadulano
force-pushed
the
pyroot-tmemoryregulator-destruction-order
branch
from
7edc459 to
3f53362
Compare
This was referenced Nov 7, 2024
guitargeek
approved these changes
Nov 7, 2024
Contributor
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
This is really great, and the CI also starts to be green! The failures are unrelated and caused by #16697. FYI, @ferdymercury.
Collaborator
Yep, it's because the sibling roottest PR was overlooked. (Should have been merged simultaneously) |
* Remove usage of lambda with capture of class data member, move to free functions with singleton. * Remove TMemoryRegulator class and only keep the needed functionality in RegulatorCleanup * Remove extra call to `ClearProxiedObjects`, as those objects are automatically deregistered by the Python GC through cppyy's memory regulator. The only remaining feature for the PyROOT atexit cleanup logic is the `CallCppyyRecursiveRemove` function that is still required for TObject-derived instances. Unfortunately, it is still somewhat fragile. With the changes in this commit, which bring back our full suite of CI platforms to green, the interplay between `CPyCppyy::MemoryRegulator::RecursiveRemove` and our own map of tracked objects is subtle. At the moment it looks like the call to `RecursiveRemove` may invalidate the memory pointed to by the map bucket, so we erase that first.
This avoids potential double-delete situations that might arise during the atexit cleanup logic of PyROOT.
vepadulano
force-pushed
the
pyroot-tmemoryregulator-destruction-order
branch
from
3f53362 to
4baf51d
Compare
Member
Author
|
I have added more comments to the commit descriptions. Once the CI confirms these changes indeed work I will merge. Also, we can merge the CI for ubuntu24.10 directly with this PR. |
Contributor
|
Nice! And cool to see that it also works on Fedora 41 with Python 3.13 |
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
3 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.
With all these changes, I do not see anymore failures in a local run with the ubuntu 24.10 CI image. let's see if the rest of the CI agrees. Note that one of the changes is equivalent to #16671