Add OP_REF_CMP to optimize (ref $x eq/ne 'SCALAR') patterns#24459
Add OP_REF_CMP to optimize (ref $x eq/ne 'SCALAR') patterns#24459richardleach wants to merge 3 commits into
Conversation
richardleach
added
the
defer-next-dev
|
Some numbers.
WAS NOW
WAS NOW In both "NOW" cases, |
|
Note: On the face of it, Scalar::Util notes that "Note that for internal reasons, all precompiled regexps (qr/.../) are blessed references; thus ref() returns the package name string "Regexp" on these but reftype() will return the underlying C structure type of "REGEXP" in all capitals." It looked like Since |
I would consider that a serious bug, and I don't see that behavior on 5.42.2: |
Thanks, maybe I'm misremembering. I only tried briefly to add support for |
Where we're at now: |
I will double check the behaviour of this PR here! |
|
Might be a little confusion; ref, as the subject of this PR, returns the class name (like 'Regexp') for blessed refs and internal reftype otherwise. builtin::reftype should have the same exact behavior as Scalar::Util::reftype, which is that it should only return uppercase internal "types" and not class names regardless of blessing status. |
That's also not true. (I just misremembered the contents of builtin.c all being XSUBs.) I'll take a look at:
On a related point, when should |
I don't see an explicit mention, can you confirm that this preserves the existing behaviour of punning classnames? |
They already do, or did I miss a point? |
Yes, it works: |
That's an amazing* improvement in stalled cycles. The rest of the stats scale at least very roughly with the overall improvement but this comes close to zeroing it out. * I'd say "ridiculous" but that might imply I don't believe the results. |
|
A bit of pickiness: I first read the name of the op, OP_REF_SEQNE, as "an op concerned with reference sequences not equal to". Perhaps shorten it to OP_REF_EQ or OP_REF_CMP say, to indicate that a ref is being compared to something? |
|
Awesome improvement. Question: will this affect |
|
Yes I'm with @iabyn on this. The name doesn't quite feel right - something like |
|
Thanks for the feedback, folks. In no particular order:
|
Stalled frontend cycles seems to be the noisiest metric, greatly influenced by what else the machine is doing at the time. I have an incling it's mostly noise here. |
923aedd to
75c2d75
Compare
|
OP now renamed, copy-pasta error in t/op/ref.t fixed, redundant code in Supporting |
I see a non-trivial amount of this on CPAN in a cursory search. |
It can be seen as an interesting defence against |
|
@leonerd - Asking 'cos it's causing problems with adding support for Just checking if I need to figure out how to work with that, or if we can just knock off the folding flag for |
75c2d75 to
4317189
Compare
01f2e78 to
ff6d4af
Compare
ff6d4af to
e919ee3
Compare
richardleach
changed the title
e61f034 to
e21ff04
Compare
leonerd
removed
the
defer-next-dev
|
Added a final commit for the perldelta entry. |
Finding value of `ref`, comparing it to a specific one of Perl's
builtin types, and doing something with that boolean result is
currently a fair bit of work.
if (ref $x eq 'SCALAR) { # do something }
or
if (ref $x ne 'SCALAR) { # do something }
The type of `$x` has to be determined and the string value for it
pushed onto the stack, the value to compare it to is also pushed,
a string comparison is then performed, with `&PL_sv_yes` or
`&PL_sv_no` put on the stack to be consumed by a LOGOP in order to
determine the control flow.
The optree looks something like this:
4 <;> nextstate(main 2 -e:1) v:{ ->5
- <1> null vK/1 ->d
9 <|> and(other->a) vK/1 ->d
8 <2> seq sKP/2 ->9
6 <1> ref[t2] sK/1 ->7
5 <0> padsv[$x:1,2] s ->6
7 <$> const[PV "SCALAR"] s/BARE ->8
This commit instead adds an OP_REF_CMP optimization, generating
an optree like this instead:
4 <;> nextstate(main 2 -e:1) v:{ ->5
- <1> null vK/1 ->b
7 <|> and(other->8) vK/1 ->b
6 <1> ref_cmp sKP/SCALAR,SKIPLOGOP,AND ->7
5 <0> padsv[$x:1,2] s ->6
OP_REF_CMP determines the type of the operand, more efficiently
compares it to the desired type (baked into the OP's flags),
and directly calls the `op_other` or `op_next` of the LOGOP.
The existing classname handling behaviour is preserved.
$x = bless {}, "ARRAY";
if (ref $x eq "ARRAY") {} # This *is* still true.
This commit also optimizes the same sort of patterns that use `reftype`
instead of `ref`. The behaviour differences between the two are
preserved.
As there were spare bits available, some additional common comparators
have also been baked in:
* `Regexp` - for the `qr//` case with `ref`
* `''` - the empty string
To avoid duplication of "what reference type is this?" logic across `Perl_sv_reftype` and `pp_ref_cmp`, this commit: * Renames `OPpREF_CMP_*` constants to `SVrt_*` equivalents * Adds a `PL_sv_reftype_lookup` table ordered to match those constants * Adds a `Perl_sv_reftype_id` function that takes the logic from `Perl_sv_reftype` but returns only an index ID value * `Perl_sv_reftype` uses that new function and table to return strings * `pp_ref_cmp` uses the function and compares to the expected ID value
2b67e1e to
3ba1f94
Compare
|
Now with a bump to lib/B/Op_private.pm |
9 participants
Finding value of
ref, comparing it to a specific one of Perl's builtin types, and doing something with that boolean result is currently a fair bit of work.or
The type of
$xhas to be determined and the string value for it pushed onto the stack, the value to compare it to is also pushed, a string comparison is then performed, with&PL_sv_yesor&PL_sv_noput on the stack to be consumed by a LOGOP in order to determine the control flow.The optree looks something like this:
This commit instead adds an OP_REF_SEQNE optimization, generating an optree like this instead:
OP_REF_SEQNE determines the type of the operand, more efficiently compares it to the desired type (baked into the OP's flags), and directly calls the
op_otherorop_nextof the LOGOP.Closes #23395