Skip to content

Dedicated SV copying 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.

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 14 commits into
base: blead
Choose a base branch
from

Conversation

richardleach
Copy link
Contributor

@richardleach richardleach commented Apr 15, 2025

Perl_sv_setsv_flags is the heavyweight function for assigning the value(s) of
a source SV to a destination SV. It contains many branches for preparing the
destination SV prior to assignment. However:

  • If the destination SV has just been created, much of that logic isn't needed.
  • When cloning a SV, simple assignments (particularly IVs and PVs) dominate.

This set of commits:

  • Extracts the "is this CoWable?" test from Perl_sv_setsv_flags into a macro.
  • Adds Perl_sv_freshcopy_flags and two static helper functions.
  • Modifies Perl_newSVsv_flags and Perl_sv_mortalcopy_flags to use them.
  • Standardizes a number of call sites that did their own things but really
    should use Perl_newSVsv_flags or Perl_sv_mortalcopy_flags.

Using perl's test harness as a guide:

  • Bodyless code handles 45% of calls to Perl_newSVsv_flags and
    57% of calls to Perl_sv_mortalcopy_flags.
  • The SVt_PV/SVp_POK code handles 32% of calls to
    Perl_newSVsv_flags and 36% of calls to Perl_sv_mortalcopy_flags.
  • S_sv_freshcopy_flags code handles 95% of the remainder in
    Perl_newSVsv_flags and 91% of the remainder in to Perl_sv_mortalcopy_flags.

With these changes compared with a build of blead:

  • perl -e 'for (1..100_000) { my $x = [ (1) x 1000 ]; }' runs 10% faster

  • perl -e 'for (1..100_000) { my $x = [ ("Perl") x 250 ]; }' runs 45% faster


  • This set of changes does require a perldelta entry and has one.
@richardleach richardleach added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Apr 15, 2025
@Leont
Copy link
Contributor

Leont commented Apr 29, 2025

Cloning is rather unfortunate choice of words, given that it has a very specific meaning in our codebase that is quite different from what this PR is about. Renaming the PR may be helpful.

@richardleach richardleach changed the title Dedicated SV cloning code in place of Perl_sv_setsv_flags Dedicated SV copying code in place of Perl_sv_setsv_flags Apr 29, 2025
@richardleach richardleach force-pushed the S_sv_freshcopy_flags branch 2 times, most recently from c392526 to 91c2b99 Compare May 8, 2025 16:54
@richardleach
Copy link
Contributor Author

I've made a lot of changes following earlier comments - thanks for those - and have finally force-pushed.

These changes aren't complete. For example:

  • Measured performance seems worse than in the PR version, so I need to look into that
  • Might change sflag handling/ SvFLAGS(dsv) setting
  • Not settled on struct membet initialisation
  • Might still rename the function that is currently Perl_newSVsv_flags and have newSVsv_flags be a macro that checks (ssv) before calling the sv.c function.
@richardleach
Copy link
Contributor Author

Rebased to clear merge conflicts.

Since around the 5.10 era, `Perl_sv_setsv_flags` has unconditionally set
`SvPOK_only(dsv)` in the `SVp_POK` branch. The associated comment reads:

    /* Whichever path we take through the next code, we want this true,
       and doing it now facilitates the COW check.  */

Things have changed since 5.10 though, in particular using the `‎SVf_POK`
to distinguish between a value that started off as a string from one that
was originally an integer/float and later stringified.

This commit:
* Removes the `SvPOK_only(dsv)` in favour of `SvOK_off(dsv)` and hoisting
  the copying of `sflags` over.
* Transforms the subsequent now-redundant `SVf_POK` toggles into asserts
  (to help reduce) the chance of inadvertent behaviour changes.
`Perl_sv_setsv_flags` is a hot function that contains liberal sprinklings
of `SvOK_off()`. This commit changes two instances, where the operand SV
cannot possibly be using the OOK hack, to do direct flag twiddling instead.

`SvOK_off()` does two things:
1. Toggles off some flags:
    SvFLAGS(sv) &= ~(SVf_OK|SVf_IVisUV|SVf_UTF8)

2. Checks for use of the OOK hack and undoes it:
    ((void)(SvOOK(sv) && (sv_backoff(sv),0)))

At least some compilers seem to struggle to figure out when `SvOOK(sv)`
cannot be true and to then elide the call to `sv_backoff()`. This is
desirable when:

1. ssv & dsv are both lower types than SVt_PV and cannot support OOK
2. inside a block following a conditional check that OOK is not in use

In the two cases identified, the flag toggling is now done explicitly.
Perl_newSVsv_flags_NN creates a fresh SV that contains the values of its
source SV argument. It's like calling `new_SV(dsv)` followed by
`sv_setsv_flags(dsv, ssv, flags`, but is optimized for a brand new
destination SV and the most common code paths.

The intended initial users for this new function were:
* Perl_sv_mortalcopy_flags (still in sv.c)
* Perl_newSVsv_flags (now a simple function in sv_inline.h)

Perl_newSVsv_flags_NN prioritises the following hot cases:
* SVt_IV containing an IV
* SVt_IV containing an RV
* SVt_NV containing an NV
* SVt_PV containing a PV

It will then check for:
* SVt_NULL
* SVt_IV containing a UV
* SVt_LAST

The helper function S_newSVsv_flags_NN_PVxx is called for everything else.
It will use Perl_sv_setsv_flags as a fallback for rare or tricky cases.

S_newSVsv_flags_NN_POK is a dedicated helper for string swipe/COW/copy
logic and is called from both Perl_newSVsv_flags_NN and
S_newSVsv_flags_NN_PVxx.

With these changes compared with the previous commit:

* `perl -e 'for (1..100_000_0) { my $x = { (1) x 1000 }; }'` runs about 20% faster

* `perl -e 'for (1..100_000_0) { my $x = { ("Perl") x 250 }' runs about 40% faster

* `perl -e 'for (1..100_000_0) { my $x = { a => 1, b => 2, c => 3, d => 4, e => 5 }; }'`
   is a touch faster, but within the margin for error

* `perl -e 'for (1..100_000_0) { my $x = { a => "Perl", b => "Perl", c => "Perl", d => "Perl", e => "Perl" } ; }'`
   runs about 17% faster
Perl_newSVsv_flags has become just a stub around Perl_newSVsv_flags_NN.
For callers where the source SV* is NULL, not having to call a
function in sv.c to immediately return is very desirable.
Besides using the just-introduced faster path for SV copying, this
allows the check for SV_GMAGIC to be pushed into the called function
without having to worry about SV leaks.

Two additional micro-optimizations are also in this commit:
* A pointer to xav_fill is cached for use in the loop. This can
  be used directly to update AvFILLp(av), rather than having to
  get there from av's SV* each time.

* The value of the loop iterator, i, is directly written into
  xav_fill, rather than getting the value in that slot,
  incrementing it (to get the same value as i), and writing it back.
@richardleach richardleach force-pushed the S_sv_freshcopy_flags branch from 9d01e85 to 7becff8 Compare August 9, 2025 23:45
@richardleach
Copy link
Contributor Author

Rebased for perldelta conflict.

Ready for any final reviews.

@tonycoz
Copy link
Contributor

tonycoz commented Aug 13, 2025

the last commit's message indicates it should be squashed

SvCUR_set(dsv, 0);
#endif
sv_grow_fresh(dsv, cur + 1);
Move(SvPVX_const(ssv),SvPVX(dsv),cur,char);
Copy link
Contributor

Choose a reason for hiding this comment

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

sv_grow_fresh has a retval, use it. dont write SvPVX(dsv)

#endif
sv_grow_fresh(dsv, cur + 1);
Move(SvPVX_const(ssv),SvPVX(dsv),cur,char);
*(dsv->sv_u.svu_pv + cur) = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

my pref is to always do the Perl '\0' safety assignment right before the Move() or Copy(), because sv_u.svu_pv and cur have already been "lifted" and are close at hand, Remember they still need to belifted and then duplicated to be outgoing args to Move(). This removes the dsv-> read opcode

Lifted is my generic word, if its C stack/non vol reg in ASM, we dont care, this is Asm-in-C, not Asm-in-Asm, we are not name dropping OSes or CPU ISA.pdfs here or ABI docs here

SvFLAGS(dsv) = SVt_PV;
SvRV_set(dsv, NULL);
}
SvCUR_set(dsv, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

SvRV_set(dsv, NULL); SvRV_set(dsv, SvREFCNT_inc(SvRV(ssv))); factor out the 2 SvRV_set() to after the if/else braches but b4 SvCUR_set(dsv, 0);, use a temp_sv SV* if need to transport the NULL const and and the SvREFCNT_inc(SvRV(ssv)) ptr to the now 1, SvRV_set() macro.

Move SvREFCNT_inc(SvRV(ssv)) to right below if (SvROK(ssv) ) {we want the CC to dedup the SvFLAGS(dsv) = assignments They cant have any combine deduping opportunities by the CC if SvREFCNT_inc(SvRV(ssv)) is in the way

SvCUR_set(dsv, 0);
SvLEN_set(dsv, 0);
SvPV_set(dsv, NULL);
return dsv;
Copy link
Contributor

Choose a reason for hiding this comment

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

        case SVf_ROK:  /* [ 3% ]*/
            SvRV_set(dsv, SvREFCNT_inc(SvRV(ssv)));
            SvCUR_set(dsv, 0);
            SvLEN_set(dsv, 0);
            return dsv;
            SvCUR_set(dsv, 0);
            SvLEN_set(dsv, 0);
            SvPV_set(dsv, NULL);
            return dsv;

make these 2 tails to be in identical assignment order so a CC coulde dedupe them, bottom quote sets slot 4 last, top quote sets slot 4 first.

assert( &(dsv->sv_u.svu_iv)
== &(((XPVIV*) SvANY(dsv))->xiv_iv));
dsv->sv_u.svu_iv = old->sv_u.svu_iv;
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there so much hate for the step-child of the sv.c API, its name is SVf_IVisUV. aren't SvIVX/SvUVX same length, 2s complements overlapping slots with no risk of sign extension

SET_SVANY_FOR_BODYLESS_IV(dsv);
dsv->sv_u.svu_iv = old->sv_u.svu_iv;
SvFLAGS(dsv) = SVt_IV|SVf_IOK|SVp_IOK|SVf_IVisUV;
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

see note above about IVX and UVX fields being bitpattern size alighment union overlapping, why have 2 branches, one for IV only and other branch for UV?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants
close