Skip to content

Implement GH-10854: TSRM should set a smarter value for expected_threads #10867

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

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

nielsdos
Copy link
Member

Closes GH-10854

The tsrm_startup() function is currently always called with expected_threads = 1. This means that the hashtable used in the TSRM will only contain a single bucket, and all thread resources will therefore be in the same linked list. So it's not really a hashtable right now, even though it's supposed to be.

This patch adds a function tsrm_startup_ex() which takes the expected thread count as an argument. It also keeps the tsrm_startup() function so there are no BC breaks.

In the Apache SAPI we query how many threads we have, and pass that to the tsrm_startup_ex() function.

…hreads

The tsrm_startup() function is currently always called with expected_threads = 1.
This means that the hashtable used in the TSRM will only contain a single bucket,
and all thread resources will therefore be in the same linked list.
So it's not really a hashtable right now, even though it's supposed to be.

This patch adds a function tsrm_startup_ex() which takes the expected
thread count as an argument. It also keeps the tsrm_startup() function
so there are no BC breaks.

In the Apache SAPI we query how many threads we have, and pass that to
the tsrm_startup_ex() function.
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I wonder if allocating a higher value "just in case" would make sense, as this allocation is unique and very small.

@nielsdos
Copy link
Member Author

The current value should be the maximum number of threads per children. So I think that's good for the capacity of the hashtable.
Do you mean to increase it just to prevent collisions? In that case it's probably better if we choose the closest prime number >= the expected_threads count. But that would be something to implement in TSRM itself instead of Apache.

@iluuu1994
Copy link
Member

I meant increasing it for the other SAPIs, but I don't know if that makes sense.

@nielsdos
Copy link
Member Author

I checked the other SAPIs:

  • CGI, CLI, PHPDBG: only run 1 thread, so nothing needs to change
  • Fuzzer: not important to optimize the table, so nothing needs to change
  • Embed: depends on the embedder, we could provide something in which they can configure it I guess, but maybe that's for another PR
  • FPM: comment on top of file says it only runs in 1 thread, so nothing needs to change

So we only need to think some more about embed probably :-). I haven't spent time looking at that SAPI yet; and I think that would be for another PR.

@nielsdos nielsdos merged commit 4da0da7 into php:master Mar 17, 2023
@iluuu1994
Copy link
Member

Right, so this looks fine then 🙂

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.

TSRM should set a smarter value for expected_threads
2 participants