Skip to content

Dedicated SV cloning code in place of Perl_sv_setsv_flags #23202

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: blead
Choose a base branch
from
Open

Dedicated SV cloning code in place of Perl_sv_setsv_flags #23202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
68c014c
Extract S_SvPV_shared_hkey_or_CoWable logic from Perl_sv_setsv_flags
richardleach Apr 3, 2025
3a2cf3b
sv.c: Add S_sv_freshcopy_flags and S_sv_freshcopy_POK
richardleach Apr 1, 2025
27bfb77
Perl_av_make - use newSVsv_flags rather than doing own thing
richardleach Apr 3, 2025
dfaf1f0
Perl_sv_recode_to_utf8 - do mortalcopy directly
richardleach Apr 4, 2025
dcab4dd
pp_push - do newSVsv_flags directly
richardleach Apr 4, 2025
d7e2159
S_doopen_pm - make mortalcopy directly
richardleach Apr 4, 2025
b3b192b
S_require_file - make mortalcopy directly
richardleach Apr 4, 2025
24b40aa
pp_subst - make mortalcopy directly
richardleach Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions av.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,9 +493,7 @@ Perl_av_make(pTHX_ SSize_t size, SV **strp)

SvGETMAGIC(*strp); /* before newSV, in case it dies */
AvFILLp(av)++;
ary[i] = newSV_type(SVt_NULL);
sv_setsv_flags(ary[i], *strp,
SV_DO_COW_SVSETSV|SV_NOSTEAL);
ary[i] = newSVsv_flags(*strp,SV_DO_COW_SVSETSV|SV_NOSTEAL);
strp++;
}
/* disarm av's leak guard */
Expand Down
10 changes: 6 additions & 4 deletions pp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6421,10 +6421,12 @@ PP(pp_push)
PL_delaymagic = DM_DELAY;
for (++MARK; MARK <= PL_stack_sp; MARK++) {
SV *sv;
if (*MARK) SvGETMAGIC(*MARK);
sv = newSV_type(SVt_NULL);
if (*MARK)
sv_setsv_nomg(sv, *MARK);
if (*MARK) {
SvGETMAGIC(*MARK);
sv = newSVsv_flags(*MARK, SV_DO_COW_SVSETSV);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe cache *MARK to a auto and skip the 2nd -O1/2 deref in sv = newSVsv_flags(*MARK, SV_DO_COW_SVSETSV);? fn call containing macro SvGETMAGIC(*MARK); won't overwrite the SV* on the PL stack with a new SV*.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just calling newSVsv_flags(*MARK, SV_DO_COW_SVSETSV|SV_GMAGIC) should be better

            if (*MARK) {
                sv = newSVsv_flags(*MARK, SV_DO_COW_SVSETSV|SV_GMAGIC);
            }
            else
                sv = newSV_type(SVt_NULL);

does (MARK is %rbx), %rdi is the first function argument:

        .loc 2 6424 13 view .LVU16810
        .loc 2 6424 17 is_stmt 0 view .LVU16811
        movq    (%rbx), %rdi
        .loc 2 6424 16 view .LVU16812
        testq   %rdi, %rdi
        jne     .L5826
        .loc 2 6428 17 is_stmt 1 view .LVU16813
        ; the else with "sv = newSV_type(SVt_NULL);", not shown
...
.L5826:
.LBB7306:
        .loc 2 6425 17 view .LVU16801
        .loc 2 6425 22 is_stmt 0 view .LVU16802
        movl    $1538, %esi
        call    Perl_newSVsv_flags@PLT

Perl_newSVsv_flags() already has the GMAGIC test and mg_get() call, saving space and a duplicate test here, and the source SV goes into a register and stays there.

} else {
sv = newSV_type(SVt_NULL);
}
av_store(ary, AvFILLp(ary)+1, sv);
}
if (PL_delaymagic & DM_ARRAY_ISA)
Expand Down
9 changes: 3 additions & 6 deletions pp_ctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4407,13 +4407,11 @@ S_doopen_pm(pTHX_ SV *name)
return NULL;

if (memENDPs(p, namelen, ".pm")) {
SV *const pmcsv = sv_newmortal();
PerlIO * pmcio;
SV *const pmcsv = sv_mortalcopy_flags(name, SV_GMAGIC|SV_NOSTEAL|SV_DO_COW_SVSETSV);

SvSetSV_nosteal(pmcsv,name);
sv_catpvs(pmcsv, "c");

pmcio = check_type_and_open(pmcsv);
PerlIO * pmcio = check_type_and_open(pmcsv);
if (pmcio)
return pmcio;
}
Expand Down Expand Up @@ -4805,8 +4803,7 @@ S_require_file(pTHX_ SV *sv)
}

if (SvPADTMP(nsv)) {
nsv = sv_newmortal();
SvSetSV_nosteal(nsv,sv);
nsv = sv_mortalcopy_flags(sv, SV_GMAGIC|SV_NOSTEAL|SV_DO_COW_SVSETSV);
}

const char *method = NULL;
Expand Down
3 changes: 1 addition & 2 deletions pp_hot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5357,8 +5357,7 @@ PP(pp_subst)
if (dstr) {
/* replacement needing upgrading? */
if (DO_UTF8(TARG) && !doutf8) {
nsv = sv_newmortal();
SvSetSV(nsv, dstr);
nsv = sv_mortalcopy_flags(dstr, SV_GMAGIC|SV_DO_COW_SVSETSV);
Copy link
Contributor

@bulk88 bulk88 Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-think this chunk, macro SvSetSV macro has an ultra fast shortcut/bypass in it. That shortcut will be lost if this chinkgoes to production. sv_mortalcopy_flags is a linkable extern C func. The macro's shortcut path doesn't have any fn calls in it. The shortcut branch in that macro may or may not be executable in real life at this line of code. Plz research.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shortcut/bypass:

#define SvSetSV_and(dst,src,finally) \
        STMT_START {                                    \
            SV * src_ = src;                            \
            SV * dst_ = dst;                            \
            if (LIKELY((dst_) != (src_))) {             \
                sv_setsv(dst_, src_);                   \
                finally;                                \
            }                                           \
        } STMT_END
...
#define SvSetSV(dst,src) \
                SvSetSV_and(dst,src,/*nothing*/;)

dst_ is never equal to src_ here (unless dst_ points to freed SV, which would be a serious bug and shouldn't be optimized for).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shortcut/bypass:
....
dst_ is never equal to src_ here (unless dst_ points to freed SV, which would be a serious bug and shouldn't be optimized for).

@tonycoz you are correct, That macro's optimization branch is impossible to execute at this particular line.

sv_utf8_upgrade(nsv);
c = SvPV_const(nsv, clen);
doutf8 = TRUE;
Expand Down
Loading
Loading