- Notifications
You must be signed in to change notification settings - Fork587
Perl_leave_adjust_stacks: additional efficiency for mortal copies#22163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This can be 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm certainly happy to write it like that. The current form is just mirroring what's already been in use inPerl_sv_setsv_flags.
Anyone else want to weigh in on preprocessor policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, making those changes as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Er, stuff has gone awry in tests, will have to look into it later.
Uh oh!
There was an error while loading.Please reload this page.
c717a64 to8e27883Comparepp_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks Tony. I'll revert to the original PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The SvTYPE() comparison that mauke suggested is still fine, it just doesn't work for this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 allother types, a new SVt_NULL is passed into sv_setsv_flags, where it willbe 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.
8e27883 tof40ea39CompareI'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.