Open
Conversation
morrisonlevi
changed the title
|
✨ Fix all issues with BitsAI or with Cursor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3670 +/- ##
=======================================
Coverage 62.08% 62.08%
=======================================
Files 141 141
Lines 13352 13352
Branches 1746 1746
=======================================
Hits 8290 8290
Misses 4269 4269
Partials 793 793 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2026-02-25 13:22:47 Comparing candidate commit e26db1d in PR branch Found 2 performance improvements and 6 performance regressions! Performance is the same for 186 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing
scenario:HookBench/benchHookOverheadTraceMethod-opcache
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4
|
morrisonlevi
changed the title
morrisonlevi
force-pushed
the
levi/cache-getenv
branch
from
20750ac to
5a292ea
Compare
In rinit, calling system getenv when a SAPI env var isn't found is slow enough it shows up in profiles. glibc's getenv does a linear scan on the environ and does strncmp on the members, so we can definitely do better. This is also arguably more correct: system environment variables map to a system INI setting, which shouldn't change at runtime.
morrisonlevi
force-pushed
the
levi/cache-getenv
branch
from
5a292ea to
e26db1d
Compare
bwoebi
reviewed
Feb 27, 2026
| (void)in_request; | ||
| #endif | ||
|
|
||
| if (in_request) { |
Collaborator
There was a problem hiding this comment.
We only need it for FPM, right? So let's add a sapi_module check here?
Saving some minor time on CLI scripts.
bwoebi
reviewed
Feb 27, 2026
| if (!cached->ptr) return false; | ||
| if (cached->len >= buf.len) return false; | ||
|
|
||
| memcpy(buf.ptr, cached->ptr, cached->len + 1); |
Collaborator
There was a problem hiding this comment.
There's not really an intrinsic need to copy. The copying in zai_getenv_ex is also unnecessary, but here we very clearly don't need it (but yeah, you'll have to duplicate branches then).
I'd personally return a simple zai_str instead of zai_env_buffer.
bwoebi
reviewed
Feb 27, 2026
Comment on lines
+15
to
+22
| typedef struct { | ||
| // ptr == NULL means env var was unset at MINIT. | ||
| // ptr != NULL with len == 0 means env var was set to an empty string. | ||
| char *ptr; | ||
| size_t len; | ||
| } zai_config_cached_env_value; | ||
|
|
||
| static zai_config_cached_env_value zai_config_cached_env_values[ZAI_CONFIG_ENTRIES_COUNT_MAX][ZAI_CONFIG_NAMES_COUNT_MAX]; |
Collaborator
There was a problem hiding this comment.
Suggested change
| typedef struct { | |
| // ptr == NULL means env var was unset at MINIT. | |
| // ptr != NULL with len == 0 means env var was set to an empty string. | |
| char *ptr; | |
| size_t len; | |
| } zai_config_cached_env_value; | |
| static zai_config_cached_env_value zai_config_cached_env_values[ZAI_CONFIG_ENTRIES_COUNT_MAX][ZAI_CONFIG_NAMES_COUNT_MAX]; | |
| static zai_string zai_config_cached_env_values[ZAI_CONFIG_ENTRIES_COUNT_MAX][ZAI_CONFIG_NAMES_COUNT_MAX]; |
Simply?
Collaborator
Author
There was a problem hiding this comment.
zai_string isn't supposed to have a null pointer, right? IIRC that's why I did this. This distinguishes between unset and null. But there is a zai_option_str, and I think using a zai_option_str except actually owning it is better than putting null into zai_string. So I'll experiment with that.
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.
Background
In request initialization aka rinit/activate, calling the system
getenvwhen a SAPI env var isn't found is slow enough it shows up in profiles:Note that glibc's
getenvdoes a linear scan on the environ and doesstrncmpon the members, so we can definitely do better.Description
Caches libc's
getenvin minit and again in first rinit. Note that php-fpm'senvdirective actually changes the system env var, it doesn't include it in the fastcgi environment. This change is also more correct: system environment variables map to a system INI setting, which shouldn't change at runtime, so if they do, we shouldn't really respect it.Here's an example diff from the profiling comparison view:
Reviewer checklist