toke.c dont call libc's memcmp() to test 1 byte in Perl_scan_str() #23533
Conversation
delim_byte_len is almost always 1, open_delim_str is almost always '"' or '\'' or something similar. I'm not sure which exact string of PP code will make delim_byte_len not be 1, but it would be too rare to optimize for but still must be supported. Just test the char directly if its length of 1. Invoking libc memcmp() requires 4 ABI inputs on any CPU, and while most of the code paths above the memEQ() lines are constants directly initialized inside Perl_scan_str(), one branch uses "utf8_to_uv_or_die(,,&delim_byte_len)" which optimizes to Perl_utf8_to_uvchr_buf_helper(,,,&delim_byte_len) making the value in STRLEN delim_byte_len unbounded according to all CC. All CCs must assume the value Perl_utf8_to_uvchr_buf_helper() put inside delim_byte_len could be a 4.7GB DVD or 25GB BD .iso file. Put the retval of SvGROW() to use. Don't let C auto var delim_byte_len escape with "&" op thru utf8_to_uv_or_die(). Var delim_byte_len can never be stored in a register again by any CC if it escapes and must be reread from C stack after ever possible call if it escapes.
There was a problem hiding this comment.
My primary issue with this is that "What is it about this code that makes it different from other uses of memEQ in general.?" Or rather, "If this is a good idea here, why not do it in memEQ and get the benefits everywhere?" And for that matter, how do we know that memcmp isn't an inline function that does the same thing? I have looked at various libc sources in the past, and recall this sort of thing being done. It's a reasonable thing to do
The commit title is fine, and the message is better than recent ones, but I don't think it is good enough. There are misspellings and short cuts like "thru" which may not be known except to native English speakers. The message must be easily comprehensible to people without the specialized knowledge it assumes, nor the jargon.
It also is wrong. Perl_utf8_to_uvchr_buf_helper is only called in unusual circumstances. And so all that second paragraph that I don't understand is based on false premises.
| /* set 'to' to the next character in the sv's string */ | ||
| to = SvPVX(sv)+SvCUR(sv); | ||
| to = pv + SvCUR(sv); |
There was a problem hiding this comment.
This is good, but I think it would be better in a separate commit; one that wouldn't be reverted if the rest turned out to be.
| { | ||
| && ((delim_byte_len == 1 | ||
| ? (s[1] == open_delim_str[0] | ||
| || s[1] == close_delim_str[0]) |
There was a problem hiding this comment.
I'm unhappy that this and elsewhere reformats code that corresponds to Perl Best Practices to not conform.
|
It occurred to me that maybe there could be a memEQ_multi_byte_likely() to avoid the extra conditional for cases where we think it more likely that a full memcmp is required The maximum length returned to advance is 14. Would an ASSUME in the inline function tell the compilers they don't have to plan for a full Size_t value. We could also change the parameter on the internal-only helper function so that it is a U8. |
delim_byte_len is almost always 1, open_delim_str is almost always '"' or ''' or something similar. I'm not sure which exact string of PP code will make delim_byte_len not be 1, but it would be too rare to optimize for but still must be supported.
Just test the char directly if its length of 1. Invoking libc memcmp() requires 4 ABI inputs on any CPU, and while most of the code paths above the memEQ() lines are constants directly initialized inside Perl_scan_str(), one branch uses "utf8_to_uv_or_die(,,&delim_byte_len)" which optimizes to Perl_utf8_to_uvchr_buf_helper(,,,&delim_byte_len) making the value in STRLEN delim_byte_len unbounded according to all CC. All CCs must assume the value Perl_utf8_to_uvchr_buf_helper() put inside delim_byte_len could be a 4.7GB DVD or 25GB BD .iso file.
Put the retval of SvGROW() to use.
Don't let C auto var delim_byte_len escape with "&" op thru utf8_to_uv_or_die(). Var delim_byte_len can never be stored in a register again by any CC if it escapes and must be reread from C stack after ever possible call if it escapes.