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

gh-103092: isolate _lsprof #115130

Closed
wants to merge 5 commits into from
Closed

gh-103092: isolate _lsprof #115130

wants to merge 5 commits into from

Conversation

aisk
Copy link
Member

@aisk aisk commented Feb 7, 2024

The main change is moving the static variables random_value and random_stream in rotatingtree.c to a struct and storing it in the module's state.

Other changes are to pass the struct from the module's state.

@erlend-aasland
Copy link
Contributor

Thanks; can you split out the Argument Clinic adaption and contribute that as a separate PR?

@aisk
Copy link
Member Author

aisk commented Feb 10, 2024

Sure, see #115242 .

@aisk aisk marked this pull request as draft February 10, 2024 12:17
@aisk
Copy link
Member Author

aisk commented Feb 11, 2024

Hi @erlend-aasland, I have a new thought. This change is just to isolate these two static variables:

static unsigned int random_value = 1;
static unsigned int random_stream = 0;

But as they are the state of a pseudo-random generator, they can be shared between interpreters under a lock. For single interpreter usage, there is no noticeable performance decrease. And this work can be done easily.

I have no idea if this approach is acceptable, or whether we should continue with the previous work. If it does, I can do some benchmarking to see if there is a performance decrease in multiple isolated interpreters usage.

@erlend-aasland
Copy link
Contributor

Sounds reasonable. Go ahead!

@erlend-aasland
Copy link
Contributor

Superseded by #115301

@aisk aisk deleted the isolate-lsprof branch March 1, 2024 05:24
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.

2 participants