Perl_leave_adjust_stacks: additional efficiency for mortal copies #22163
Conversation
|
This can be defer-next-dev. |
tonycoz
added
the
defer-next-dev
pp_hot.c
Outdated
| #if NVSIZE <= IVSIZE | ||
| if (SvTYPE(sv) <= SVt_NV) { | ||
| #else | ||
| if (SvTYPE(sv) <= SVt_IV) { | ||
| /* arg must be one of undef, IV/UV, or RV: skip | ||
| * sv_setsv_flags() and do the copy directly */ | ||
| #endif |
There was a problem hiding this comment.
I don't know what our policy on preprocessor use is, but this could be written more compactly as
if (SvTYPE(sv) <= (NVSIZE <= IVSIZE ? SVt_NV : SVt_IV)) {There was a problem hiding this comment.
I'm certainly happy to write it like that. The current form is just mirroring what's already been in use in Perl_sv_setsv_flags.
Anyone else want to weigh in on preprocessor policy?
There was a problem hiding this comment.
I guess the only concern would be C compilers which can't do simple constant folding, which I'm assuming would be rare these days.
There was a problem hiding this comment.
The C standard requires constant folding.
A constant expression can be evaluated during translation rather than runtime, and accordingly may be used in any place that a constant may be.
In particular, you can use constant expressions as array sizes, case labels, bit-field sizes, and null pointer constants. If it doesn't constant fold, it's not a C compiler. :-)
There was a problem hiding this comment.
Ok, making those changes as suggested.
There was a problem hiding this comment.
Er, stuff has gone awry in tests, will have to look into it later.
richardleach
force-pushed
the
hydahy/leave_adjust_copies
branch
from
c717a64 to
8e27883
Compare
pp_hot.c
Outdated
| else if (NVSIZE <= IVSIZE && (srcflags & SVf_NOK)) { | ||
| SET_SVANY_FOR_BODYLESS_NV(newsv); | ||
| dstflags = (SVt_NV|SVf_NOK|SVp_NOK|SVs_TEMP); | ||
|
|
||
| /* both src and dst are <= SVt_MV, so sv_any points to the | ||
| * head; so access the head directly | ||
| */ | ||
| assert( &(sv->sv_u.svu_nv) | ||
| == &(((XPVNV*) SvANY(sv))->xnv_u.xnv_nv)); | ||
| assert( &(newsv->sv_u.svu_nv) | ||
| == &(((XPVNV*) SvANY(newsv))->xnv_u.xnv_nv)); | ||
| newsv->sv_u.svu_nv = sv->sv_u.svu_nv; | ||
| } |
There was a problem hiding this comment.
Alas, this needs to use the preprocessor, since for long double or quadmath builds NV is 12-16 bytes and IV is 8 bytes on 64-bit platforms.
There was a problem hiding this comment.
Thanks Tony. I'll revert to the original PR.
There was a problem hiding this comment.
The SvTYPE() comparison that mauke suggested is still fine, it just doesn't work for this block.
There was a problem hiding this comment.
Ok, I had been thinking about keeping the two changes consistent, but have just reverted this second one.
|
Apart from the issue of the macros, I approve of this PR. |
The existing code has a fast path for copying a SVt_NULL or SVt_IV. For all other types, a new SVt_NULL is passed into sv_setsv_flags, where it will be upgraded into the required type by sv_upgrade(). This commit makes two changes: 1) Special case copying a SVt_NV where possible, as sv_setsv_flags does. 2) It's safe and more efficient to directly create a new type of SVt_PVNV or below, rather than upgrade it later, so do that.
richardleach
force-pushed
the
hydahy/leave_adjust_copies
branch
from
8e27883 to
f40ea39
Compare
jkeenan
removed
the
defer-next-dev
|
I'll merge this soon if there are no further comments/suggestions. |
The existing code has a fast path for copying a
SVt_NULLorSVt_IV. For allother types, a new
SVt_NULLis passed intosv_setsv_flags, where it willbe upgraded into the required type by
sv_upgrade().This commit makes two changes:
SVt_NVwhere possible, assv_setsv_flagsdoes.SVt_PVNVor below, rather than upgrade it later, so do that.