Assorted microoptimizations and hot-path refactoring #23844
+92
−74
Conversation
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
SV bodies are Zero()ed when allocated/uprooted from an arena for use. This commit changes instances where a fresh body field is unnecessarily assigned a zero/NULL value into an assertion that the field already contains the desired value.
Prior to this commit, `newSVbool`, `newSV_true`, and `newSV_false` used `newSVsv`, but that is less efficient than directly setting up the new SV as it needs to be.
Perl_regexec_flags had these lines to first create a new SV, then assign
to it the value(s) of an existing SV:
```
reginfo->sv = newSV_type(SVt_NULL);
SvSetSV_nosteal(reginfo->sv, sv);
```
This is two calls into _sv.c_ and, if the existing SV is `SvOK`, will
incur an SV upgrade in the process. Those lines are preceded with the
comment:
`Not newSVsv, either, as it does not COW.`
However, the underpinnings of `newSVsv` and variants do support COW
nowadays, so we can now just do:
```
reginfo->sv = newSVsv_flags(sv, SV_GMAGIC|SV_NOSTEAL|SV_DO_COW_SVSETSV);
```
Rather than create an `SVt_NULL`, then immediately to `sv_upgrade` it (within the `sv_usepvn` call) to an SVt_PV, just create the SVt_PV in the first place.
Since `new_XPV()` was added, this function can handle both branches of `if (len)` without needing to call any other function. This should be slightly more efficient for both branches.
Although this appears in an UNLIKELY branch, this commit shaves off some CPU instructions and may generally help the compiler to optimise the more likely branches.
This commit: * swaps `SvLEN_set(sv, 0)` for an assertion of this default value. * Moves `SvCUR_set` and the (now-combined) flag assignment before the call to `sharepvn`, giving the compiler a better chance to combine them with the initialization (likely) inlined from `newSV_type(SVt_PV)`.
Existing comments highlighted the common case, confirmed by a _gcov_ build and run of the test harness. For that workload: * `(!hek)` is vanishingly rare * `(flags & HVhek_NOTSHARED)` isn't hit by core at all. * `(HEK_LEN(hek) == HEf_SVKEY)` was about 1% of calls. The function was refactored in light of the existing comments and that _gcov_ data. This also cut the number of resulting CPU instructions by about half on a normal gcc build.
The compiler stands a better chance of optimising the function this way around. (e.g. A gcc build reduces from 20 instructions down to 15.)
The only place I see |
tonycoz
reviewed
Oct 13, 2025
| assert(!GvCVu(gv)); | ||
| GvCV_set(gv, cv); | ||
| GvCVGEN(gv) = 0; | ||
| assert(GvCVGEN(gv) ==0); |
There was a problem hiding this comment.
nit: spacing around == inconsistent
|
Otherwise fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This PR collects a hodge-podge of miscellany noticed while working on a separate branch.
Each commit (should) either cover a single theme or concern a single function.