Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PyROOT][10454] Prevent memory hogging issue when checking for enums #11412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

etejedor
Copy link
Contributor

As reported in issue #10454, calling ClassInfo_IsEnum() repeatedly increases the memory consumption of the program. This does not happen with TEnum::GetEnum(), which can be used instead.

Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I checked in the cppyy (incl legacy) history that this isn't roundtripping from TEnum::GetEnum() => gInterpreter->ClassInfo_IsEnum() and back :-)

@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
@root-project root-project deleted a comment from phsft-bot Apr 8, 2024
As reported in issue root-project#10454, calling ClassInfo_IsEnum() repeatedly
increases the memory consumption of the program. This does not
happen with TEnum::GetEnum, which can be used instead.
Copy link

github-actions bot commented Apr 8, 2024

Test Results

    10 files      10 suites   1d 21h 52m 58s ⏱️
 2 635 tests  2 619 ✅ 0 💤 16 ❌
24 868 runs  24 779 ✅ 0 💤 89 ❌

For more details on these failures, see this check.

Results for commit d38ffc5.

♻️ This comment has been updated with latest results.

@pcanal pcanal closed this May 7, 2024
@pcanal pcanal reopened this May 7, 2024
@pcanal pcanal assigned pcanal and vepadulano and unassigned etejedor May 7, 2024
@pcanal pcanal closed this May 9, 2024
@pcanal pcanal reopened this May 9, 2024
@pcanal
Copy link
Member

pcanal commented May 9, 2024

@vepadulano Can you check why this seemingly straightforward change is making some test fail?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants