remove pointless SVPV mortal alloc in S_strip_spaces()/prototype parser #22655
Conversation
|
I don't really understand the motivation behind this PR. It seems to add a lot of complexity: not only in the static function, but in all its callers. And just to save creating a mortal SV string a couple of times per sub-with-prototype compilation? Am I missing something obvious? |
The mortal SV and malloc are being created and freed, even if the string contents are "clean" and the string contents will not change. That is a bug. Perl prototypes are never more than ~10 chars long I think. It is senseless to, for EACH prototype parsed, to use malloc buffer+SV Body+SV head+free all 3 allocs, to parse the prototype, that 99% of the time, the string contents, will not change. A reasonably tiny, 16/32/64/128 C stack char array buffer is much better and faster. I left in the SVPV alloc code since the old code did it, but IMHO the SVPV alloc code should be removed and replaced by a bounds check and throw fatal parsing error. If there are no comments, I will slim down the patch, and just use a C auto 32 byte char array or fallback to SVPV mortal, everywhere. Therefore less new C code by ASCII text .c file char length. Ideally the SVPV mortal fallback code should be removed, but IDK what is the longest perl prototype that is not a grammar syntax error, and is not a security exploit/buffer overflow? |
Probably Web::Simple match specification or similar: Those can be given as subroutine prototypes: |
Examples in the POD seem to max out at 32 chars, 80 chars is line limit in Terminal. ~120 chars is line limit in a desktop URL box before being cut off. so 32/64 chars is average? extreme but some production code does it is 128? Perl syntax/grammer valid is unlimited length until OOM from malloc? |
|
On Thu, Oct 17, 2024 at 09:40:21AM -0700, bulk88 wrote:
> I don't really understand the motivation behind this PR. It seems to add a lot of complexity: not only in the static function, but in all its callers. And just to save creating a mortal SV string a couple of times per sub-with-prototype compilation? Am I missing something obvious?
The mortal SV and malloc are being created and freed, even if the string contents are "clean" and the string contents will not change. That is a bug.
I don't see how that's a bug. I looks to me like just a minor inefficiency.
My main point remains. Unless I am misunderstanding, these temporary
buffers are only created one or twice during the *compilation* of a sub,
and for that matter, only for a sub which has a prototype specified (which
is very much a minority of subs).
So I see very little that is gained by optimising this function.
Conversely, I see a *lot* of downside, especially as it makes the callers
more complex, not just the function itself.
If you're really bothered by it, why not just make S_strip_spaces() do a
quick scan for spaces and return the original prototype string if none
found. That leaves the temp malloc just for the edge cases where the
prototype hasn't been declared in perl, but presumably via some sort of XS
route.
…--
"Strange women lying in ponds distributing swords is no basis for a system
of government. Supreme executive power derives from a mandate from the
masses, not from some farcical aquatic ceremony."
-- Dennis, "Monty Python and the Holy Grail"
|
Valid, parseable, and sane prototypes, are tiny in char len and often fit on 1 hand, most extreme cases in production would be 80 chars, because terminal length. Original commit referred to mitigating sloppy XS code with random white space. Majority of XS code will have perfect clean prototype strings. So not SV Alloc, PV Alloc, MEXTENDPUSH, memcpy(), and alot more free()s in scope _dec(), for clean strings. Even for dirty but parsable prototypes, they will be tiny in bytes. Therefore use a tiny stack buffer for dirty semi-hot path to remove overhead. Fuzzing, junk, abuse, can OOM die in newSV()/malloc() if needed, same as in prior version of the code. Use newSV(len) and POK_off, SV head is private to us, and a waste to bookkeep SVPV details. SAVEFREEPV() was not used, because previous code did mortal, and not SAVEFREEPV(), so keep using mortal. This can be changed if someone has rational to do it. 64 bytes was picked since its power of 2, and below 80, but not 16 or 32 bytes which is too small and C compiler would probably be leaving padding at the end of the array anyways. Going to 80 or 100, was discarded, to give breathing room on C stack to C compilers so on x86/x64 C compilers can emit U8 arithmitic with U8 constants to access various C autos, and the CC doesn't downgrade to U32 constants to access various local C auto. Worst case production Perl code for prototype length, is a URL argument template system on CPAN, and even urls, are unlikely to be longer than a desktop's URL bar or 80 char terminals. So 64 byte was picked. The struct provides type matching and length safety between S_strip_spaces() and its callers.
bulk88
force-pushed
the
S_strip_spaces
branch
from
aa0a0e9
to
737f8d5
Compare
|
New patch, alot less lines of text. More basic. Still gets all goals accomplished. |
|
On Fri, Oct 18, 2024 at 07:46:26AM -0700, bulk88 wrote:
New patch, alot less lines of text. More basic. Still gets all goals accomplished.
And it still doesn't address any of the points I raised. The main ones
being that:
- optimisation isn't required in such non-hot code;
- that any any optimisation in any case should not need to make every
caller of S_strip_spaces() be more complex;
- and why not just make S_strip_spaces() return the original unmodified
string if it doesn't have white space, and malloc otherwise?
…--
That he said that that that that is is is debatable, is debatable.
|
See commit message. Since I minimally know about the p5 prototype syntax grammar, is there an actual max_len to that string, and it is STILL parseable/not fatal later on? Other words, can string limit be FIXED, and that temp SVPV statement removed forever, adding a hard error, and using tiny stack buf only?
10MB, 100MB, and 1GB long prototype strings are welcome. Discussing prototype max_len verges on a Perl GOLF or anti-GOLF or fuzzing/sec exploit category.