Skip to content

Conversation

@AlliBalliBaba
Copy link
Contributor

@AlliBalliBaba AlliBalliBaba commented Oct 10, 2024

I noticed that php_import_environment_variables will lock and increasingly slow down the server as more environment variables are imported.
Since many frameworks will cache environment variables in production, the import often ends up being redundant. I think it would make sense to only import them once per thread, at least in worker mode.

Some open points:

  • How do other SAPIs optimize this? (not yet sure what exactly's happening here: litespeed fpm )
  • Should this be an optional configuration in the Caddyfile?
  • Should this also be possible in non-worker mode?

Here is a graph showing that the server slows down with an increasing amount of environment variables .
When we load the environment from a zval that's cached on the thread, the slowdown is much less significant:
graph-environment-variables
the env variables have ~40 characters each
Note that 10 threads is the 'ideal' amount of threads on my 20 core machine when PHP does minimal work

@AlliBalliBaba
Copy link
Contributor Author

worker without caching (~20 env variables)
flame-no-handles
worker with caching (~20 env variables)
flame

@AlliBalliBaba AlliBalliBaba changed the title perf: cache environment variables in worker mode perf: only import environment variables once per thread Oct 10, 2024
@AlliBalliBaba AlliBalliBaba changed the title perf: only import environment variables once per thread perf: only import os environment variables once per worker thread Oct 10, 2024
@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Oct 10, 2024

Leak should be fixed. I think caching the os environment is the expected default for worker mode. I can add it as optional to the Caddy config though

@AlliBalliBaba AlliBalliBaba marked this pull request as ready for review October 10, 2024 15:53
@dunglas
Copy link
Member

dunglas commented Oct 11, 2024

I'm in favor of both adding this cache (without an option to disable it) to mimick the FPM behavior and merging #1086.

Very good work again!

@dunglas
Copy link
Member

dunglas commented Oct 12, 2024

Can we merge this one?

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Oct 12, 2024

I think we can probably merge this. Super-globals (or auto_globals) still have some weirdness about them that I'm trying to understand. Their ->jit mechanism to only load when they are being accessed doesn't restore itself when they are destroyed (more specifically this is true for $_SERVER). But that's probably something for a different PR.

@dunglas dunglas merged commit ea7a514 into php:main Oct 15, 2024
@dunglas
Copy link
Member

dunglas commented Oct 15, 2024

Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants