toke.c - only try to shrink an SV buffer when conceivably possible #23293
Conversation
A few places within toke.c try to return unused string buffer space by
doing something like:
if (SvCUR(sv) + 5 < SvLEN(sv)) {
SvPV_shrink_to_cur(sv);
}
The rationale for the 5 is not commented upon. Maybe it's a random
thumb in the air, maybe it was 1 byte for the trailing null plus
a 4 byte pointer to account for allocation alignment?
Either way, on a platform with 8 byte pointers, that code could
try to shrink buffers when there's no realistic chance of doing
so. (The space between chunk sizes will be at least 1x PTRSIZE,
often 2x PTRSIZE at smaller sizes and then progressively more.)
For a reallocation that is at least has the potential to be
meaningful, the difference between CUR and LEN should be at least:
* 1 byte for a trailing null byte +
* 1 byte to enable the SV to be COWed +
* 1x PTRSIZE
This commit changes makes that so.
richardleach
added
the
defer-next-dev
|
#23111 is of the same theme. As with that PR, only doing 1x PTRSIZE is quite conservative and perhaps we would be happy with a larger multiple. I did a quick instrument of
|
|
Instead of repeating that logic at every callsite, why not move it into |
Yeah, that's definitely an option. There's one call in each of pp_hot.c and pp_sys.c that guard with |
|
Changes to #23290 might render this PR unnecessary. |
richardleach
removed
the
defer-next-dev
|
#23290 has made this PR unnecessary. Closing. |
2 participants
A few places within toke.c try to return unused string buffer space by doing something like:
The rationale for the
5is not commented upon. Maybe it's a random thumb in the air, maybe it was 1 byte for the trailing null plus a 4 byte pointer to account for allocation alignment?Either way, on a platform with 8 byte pointers, that code could try to shrink buffers when there's no realistic chance of doing so. (The space between chunk sizes will be at least 1x
PTRSIZE, often 2xPTRSIZEat smaller sizes and then progressively more.)For a reallocation that is at least has the potential to be meaningful, the difference between CUR and LEN should be at least:
PTRSIZEThis commit changes makes that so.