Switch to Refcounted Stack #20858
Switch to Refcounted Stack #20858
Conversation
Thanks for announcing this. Can you remove the |
|
You don't need to create a new PR Dave, in fact, you should not do so. You can and should force push to the old one. Note, you shouldn't really use a smoke-me branch for your PR, keep your topic branch for your PR separate and distinct from the smoke-me version. So for instance let's say you had followed the advice about keeping smoke-me branches distinct from PR branches and you had pushed your original PR as davem/rc_stack you might do this: The reason I say don't use a smoke-me for your PR is I believe that the smoke-me infra doesn't deal with force pushes, but i know that the PR infra on github does. The recipe above will avoid spamming github with new PRs, but still let you push new versions for smoking. |
|
And actually, I wouldn't bother with smoke-me branches at all until the github Ci is green, even then these days most folks dont bother. Bit if the Ci fails then certainly so shall the smoke-me, but CI will complete in about an hour, two max. The feedback loop on smoke-me branches is much slower. |
iabyn
force-pushed
the
smoke-me/davem/rc_stack2
branch
from
8f5afba to
9ca0fcc
Compare
|
@iabyn do you think maybe we should edit the title of this PR to something like "Switch to Refcounted Stack"? I realize that you already publicized it by this name, but really you should publicize the branch, not the PR title. Github autosets the title from the topic branch, but for perl that usually isnt a great default. |
|
I have been running some tests to install modules with this branch with PERL_RC_STACK enabled, and with latest blead as a comparison. So far I see the following modules broken with PERL_RC_STACK:
|
|
indirect uses a bunch of custom OPs, so failures there are expected. |
|
I've started some preliminary testing and together with the usual suspects I've found the following interesting failures:
|
iabyn
force-pushed
the
smoke-me/davem/rc_stack2
branch
from
f0cf813 to
a29b2af
Compare
this was a bug in the XS code that the branch helped bring to light. I expect that for non PP related XS code this will be a pattern. |
|
|
|
@iabyn we can set it up so in this branch CI runs with the define and without so you dont need that last patch. Ill look into a patch for you. (Until then it makes sense to keep it!) |
|
On Wed, Mar 01, 2023 at 06:07:29AM -0800, H.Merijn Brand wrote:
Text-CSV_XS-1.50 has been uploaded to CPAN. This is the performance
graph comparing speed of a single release over different versions of
perl. It clearly shows the performance hit (up is fast, down is slow)
It's a bit difficult to judge without vertical axis labels and/or an
indication of where the origin is.
…--
Any [programming] language that doesn't occasionally surprise the
novice will pay for it by continually surprising the expert.
-- Larry Wall
|
|
@iabyn I just dug into the IO::Socket::SSL errors a bit, and realized that the module is pure perl, yet it still segfaults. Obviously it depends on several XS modules, Net::SSLeay for instance, but they dont SEGV in test, and it does. (you can ignore the warning about source code being newer than executable. I build this from your branch a while back and have since changed branch a few times.) |
|
Text-CSV_XS-1.50-perl.txt |
|
Cpanel::JSON::XS uses SAVESTACK_POS(), from your comment in scope.c: I decided to see what would happen if I simply removed it, and all tests passed. All of the uses have the same form: I have never seen that before, and I dont know what we should do about it. Cpanel::JSON::XS is a fork of code (JSON::XS) by someone who has a history of not reacting well to internals changes, so i think we can assume there will be some gnashing of teeth over this. :-( Any idea what kind of issues we should expect by removing this? @rurban maintains the Cpanel::JSON::XS fork, maybe he has some insight here. |
|
If it will effectively be a no-op, it will be easy to put in |
|
JSON::XS added calls to Those calls were brought into Cpanel::JSON::XS in version 3.0212 by merging in changes from JSON::XS. But apparently the removals have never been brought over. |
|
Regarding HTML::Tree. The error is inconsistent, sometimes it does not occur. But when it occurs it always occurs in the same test file and test, construct_tree.t just after test 44. It also seems to require that HTML::FormatText LWP::UserAgent are installed, although i am less certain of that. pp_ctl.c: 1271 is the last line of this code: DEFSV_set is defined as |
|
As regards SAVESTACK_POS() and Cpanel::JSON::XS(), I think all their uses in that distribution are effective NOOPs. They are all of the form: remember the current stack position, then do call_sv(), process any return values, then LEAVE() (which then restores the old stack pos). But in every case, before the LEAVE the code manually resets the stack anyway, e.g. the 'SP -= items;' in encode_rv(). And if the code dies, perl does stack unwinding anyway. So NOOP. One possibility is to just remove SAVESTACK_POS() from that distribution. According to grep.metacpan.org, the only other distribution on CPAN which uses that macro is B-Hooks-XSUB-CallAsOp-0.02, which hasn't had a release since 2009, and doesn't seem to be used anywhere. The other possibility is to fix perl. When I disabled SAVESTACK_POS(), I didn't yet have any easy way to fix it. Later I added So I'm kind of inclined to say keep SAVESTACK_POS() doing assert(0) and remove it from any disgtributions which still use it. |
|
Thanks. I'll happily remove it also. Old cruft |
|
On Wed, Mar 01, 2023 at 07:03:27AM -0800, Yves Orton wrote:
@iabyn I just dug into the IO::Socket::SSL errors a bit, and realized
that the module is pure perl, yet it still segfaults. Obviously it
depends on several XS modules, Net::SSLeay for instance, but they dont
SEGV in test, and it does.
I've reduced it to a small pure perl test code. Looking into it now.
…--
The Enterprise is involved in a bizarre time-warp experience which is in
some way unconnected with the Late 20th Century.
-- Things That Never Happen in "Star Trek" #14
|
merged from JSON-XS-3.02, removed there with 4.0 requested to remove with Perl/perl5#20858
iabyn
force-pushed
the
smoke-me/davem/rc_stack2
branch
from
b7ef172 to
dec99a6
Compare
|
On Wed, Mar 01, 2023 at 12:06:04PM -0800, iabyn wrote:
I've reduced it to a small pure perl test code. Looking into it now.
Fixed by the commit: Perl_runops_wrap((): don't mortalise NULLs
…--
Indomitable in retreat, invincible in advance, insufferable in victory
-- Churchill on Montgomery
|
This function indicates whether an SV on the stack is kept alive only by
a single ref from the temps stack - which is often the case with stuff
pushed onto the stack by PP functions. Often this means that the SV can
be passed through rather then copied in places like returning from a
subroutine.
With a ref-counted stack, it's possible for a *non* SvTEMP SV to also be
stealable, if it has a refcount of 1, since that SV is being kept alive
purely by its (now refcounted) link from the stack.
So this commit adds that condition too (and also simplifies the
condition for efficiency).
In summary: originally perl (and then this function) did:
if (SvTEMP(sv) && SvREFCNT(sv) == 1) { ... steal it ... }
Now this function's condition is the equivalent of
#ifdef PERL_RC_STACK
( (SvREFCNT(sv) == 1)
|| (SvTEMP(sv) && SvREFCNT(sv) == 2))
#else
(SvTEMP(sv) && SvREFCNT(sv) == 1)
#endif
When displaying the stack with perl -Ds or -Dsv, put a vertical bar (|)
at the boundary between the reference-counted part of the stack and the
non-reference-counted part, if any.
In addition with -Dsv, for each stack show its status;
(real) all items on the stack are reference-counted
(partial real) items below si->si_stack_nonrc_base are RCed
blank no items are RCed
This commit switches to invoking S_deb_stack_n() directly, rather than
via the deb_stack_n() macro, as some compilers can't cope with the
'ifdef' amongst the calling args.
This macro was just a temporary crutch that disabled actually adjusting the reference count of things being pushed and popped off the stack, even on PERL_RC_STACK builds. It allowed the core to be converted to support a reference-counted stack as a bunch of incremental changes. This commit turns it off, enabling the stack to become actually reference-counted on PERL_RC_STACK builds. In a few commits' time, this macro will be completely eliminated from the source code.
On perls built with both -DPERL_RC_STACK and -DDEBUG_LEAKING_SCALARS, add asserts to the various old-style PUSHs() etc macros and new-style rpp_push_1() etc functions that they're operating on the right sort of stack. In particular: PUSHs() and POPs() should only be used on stacks where rpp_stack_is_rc() is false, since they don't modify the reference count of the SVs they are pushing or popping. Conversely, rpp_push_1(), rpp_popfree_1() etc should only be used on stacks where rpp_stack_is_rc() is true, since they modify the reference counts of the SVs they push and pop. On perls not compiled with PERL_RC_STACK, the rpp_ functions don't modify the reference counts, but on such builds the rpp_stack_is_rc() assertions in the rpp_ functions are disabled, so it all works out still.
on PERL_RC_STACK builds,
1) assert that the stack is reference-counted (rpp_stack_is_rc()).
2) assert the any split point (si_stack_nonrc_base) isn't above the
top of the stack.
and put it in a static function, S_pp_xs_wrap_return().
After bumping up the reference counts of the returned values,
reset si_stack_nonrc_base earlier, so that if perl dies while
decrementing the original args, all reference counts will be properly
accounted for.
Also in xs_wrap(), only bother doing
PL_markstack_ptr[0] += nargs;
in the branch where nargs > 0.
This new macro allows an old-style non-reference-counted PP-style function to be used on a PERL_RC_STACK builds. It's intended for non-core PP functions which are typically in an XS file and are used to replace the PP function of an existing op, or to be attached to a custom op. It's virtually identical to the PP_wrapped() macro, except that it uses the name as-is rather than prepending Perl_ to the name
In this test function, push a couple of stackinfos, one with a ref-counted stack, and one without. After cloning, pop the cloned stackinfos and assert that the cloned argument stacks are also appropriately RC or non-RC. This should probably have been done properly as some new tests rather than a couple of asserts, but that would have involved a lot more effort.
This macro was disabled a few commits ago. Now remove all use of it from the core. It was just a temporary crutch that disabled actually adjusting the reference count of things being pushed and popped off the stack, even on PERL_RC_STACK builds. It allowed the core to be converted to support a reference-counted stack as a bunch of incremental changes.
Explain what all the new rpp_foo() functions are for.
Turns out MSVC 1.42 doesn't like
FOO(bar,
#ifdef X
1
#else
0
#endif
);
where FOO is a macro.
This function was doing a delayed ref count decrement of all the SVs it had previously temporarily incremented, by mortalising each one. For efficiency it was just doing a Copy() of a block of SVs addresses from the argument stack to the TEMPs stack. However, the TEMPs stack can't cope with NULL pointers, while there are sometimes NULL pointers on the argument stack - in particular, while doing a map, any temporary holes in the stack are set to NULL on PERL_RC_STACK builds. The fix is simple - copy individual non-NULL addresses to the TEMPS stack rather than doing a block copy.
Copy @_ SV pointers to @db::args earlier, before pp_caller() has the chance to create any new SVs (e.g. for the package name). This will give the next commit a chance to discover and skip any freed elements in @_. This commit makes no changes to the block of code that is moved; that's saved for the next commit.
When pp_caller() aliases the elements of @_ to @db::args, it may include pointers to array elements which have been shifted away. So for example in sub f { my $self = shift; ....; croak() } it populates $DB::args[0] with the SV pointer one position below the AvARRAY() of @_. This means that the argument lists in the stack backtrace provided by croak() will include the original first arg, which is nice, but is very fragile. There is no guarantee that that SV hasn't been freed or reallocated in the meantime. This commit adds a check for each SV pointer being copied to @db::args, which NULLs it if the SV has a refcount of 0. This means that in something like: package DB: () = caller($i); my @Args = @db::args; perl will no longer panic about copying a freed SV. Note that this commit does not address the issue of the SV having already been reallocated. In the example above, some elements of @Args may contain a copy of some other random variable. However, the previous commit moved to earlier in the function, the block of code in pp_caller() which populates @db::args, to before pp_caller() allocates new SVs for things like the package name. Which means the RC==0 test added by this commit has a better chance of spotting SVs which have been freed but not yet reallocated. In summary, @db::args is still a horrible, fragile hack, but these two commits make it slightly less fragile in some circumstances.
When a sub's @_ gets abandoned (e.g. when 'goto &bar' "donates" the
current @_ to the new sub), a new AV is created at pad[0] for the old
sub. Perl alloc()'s the AvARRAY() buffer for the new AV, but didn't zero
it. This didn't used to be a problem, because @_ AVs are created
!AvREAL(), so the first thing perl did when someone tries to modify @_,
is to call av_reify() on it. And it just so happens that av_reify()
zeros out all the unused slots on the array first. So code like
$_[1] = 1;
do_something_with($_[0])
was fine.
However, on PERL_RC_STACK builds, @_ is AvREAL() by default now, so
av_reify() doesn't get called, so AvARRAY() can sometimes contain random
wild pointers.
The fix is simple: zero AvARRAY() when creating it.
This was showing as a failure in Test::Trap
An earlier commit added some asserts to the old-style and new-style
stack push/pop functions such as PUSHs() or rpp_popfree_1(). These
assert that the stack is reference-counted or not as appropriate.
These asserts were enabled only if perl was built with all of
DEBUGGING && PERL_RC_STACK && DEBUG_LEAKING_SCALARS.
This commit makes the asserts available under just
DEBUGGING && PERL_RC_STACK
Initially I was worried about the performance implications (you expect
DEBUGGING builds to be slow, but not *too* slow), but they don't seem
too bad. So by making them more likely to be enabled, they're more
likely to help people spot code that needs fixing (e.g. code still doing
POPs when the stack is reference counted and the function hasn't been
wrapped).
Add a suite of tests which check whether all the "stack not reference counted" tickets are fixed when perl is built with PERL_RC_STACK
This function is just a neater version of:
#ifdef PERL_RC_STACK
Perl_xs_wrap(aTHX_ CvXSUB(cv), cv);
#else
(void)(*CvXSUB(cv))(aTHX_ cv);
#endif
but shortly it will be extended to allow XS CVs to be flagged as
"RC-aware".
Allow an XS sub to be flagged with CVf_XS_RCSTACK, which indicates that it expects a reference-counted stack - and so pp_entersub() etc should invoke it directly rather than via xs_wrap() on PERL_RC_STACK builds. Note that there is not yet any syntax in an XS file which supports marking an XS sub as 'RC-aware', so this facility is very much in the early stages and "at your own risk", requiring the CV to manually have its CVf_XS_RCSTACK flag set.
iabyn
force-pushed
the
smoke-me/davem/rc_stack2
branch
from
14148aa to
2899bb2
Compare
There was a problem hiding this comment.
I have read through these commits, and they look reasonable to me. I think it unlikely that someone will have the time to scrutinize them all carefully. Therefore, at this point in the development cycle, I think that after a suitable period to give people a chance to comment, that these be merged
|
On Sat, Aug 12, 2023 at 04:40:30PM -0700, Karl Williamson wrote:
@khwilliamson approved this pull request.
I have read through these commits, and they look reasonable to me. I
think it unlikely that someone will have the time to scrutinize them all
carefully. Therefore, at this point in the development cycle, I think
that after a suitable period to give people a chance to comment, that
these be merged
Yeah, most of the commits are unchanged from back in March, apart from
tweaks to rebase against blead. The last few commits fix up some failures
triggered by CPAN modules (e.g. edge cases with @_ and/or @db::args), then
there are few commits to address specific points raised during earlier
review. In particular,
Added: t/op/refstack.t - this test file contains the failing code samples
from all 68 GH tickets which were part of the RT 'stack not ref counted
issues' meta ticket. All these tests pass under PERL_RC_STACK (and are
skipped on default builds).
Added support for calling RC-aware XS subs. Note that this doesn't add
support to the XS parser for flagging XS subs as RC-aware, it merely adds
a mechanism to pp_entersub() etc to call those flagged XS functions
without wrapping them.
After merging, my pain will become everyone else's pain: any new pp()
functions you write (I'm looking at you, Paul!) should be in the new
style, using rpp_push_1() rather than PUSHs() etc.
Other than that, in theory blead shouldn't be much affected on default
builds - just a few edge cases around the handling of @db::args etc have
been tweaked.
My next major piece of work on this project will be to unwrap many of the
lightweight pp() functions, such as pp_const(). This should claw back a
lot of the performance drop.
I propose that we should add an additional GH build check thinggy
configuration (i.e. all the things that are run when you push or update a
PR):
-Duseithreads -DDEBUGGING -Accflags='-DPERL_RC_STACK'
to ensure it doesn't bitrot. I don't know who is responsible for updating
such things.
…--
Hofstadter's Law: It always takes longer than you expect, even when you
take into account Hofstadter's Law.
|
|
On Mon, 14 Aug 2023, 22:07 iabyn, ***@***.***> wrote:
On Sat, Aug 12, 2023 at 04:40:30PM -0700, Karl Williamson wrote:
> @khwilliamson approved this pull request.
>
> I have read through these commits, and they look reasonable to me. I
> think it unlikely that someone will have the time to scrutinize them all
> carefully. Therefore, at this point in the development cycle, I think
> that after a suitable period to give people a chance to comment, that
> these be merged
Yeah, most of the commits are unchanged from back in March, apart from
tweaks to rebase against blead. The last few commits fix up some failures
triggered by CPAN modules (e.g. edge cases with @_ and/or @db::args), then
there are few commits to address specific points raised during earlier
review. In particular,
Added: t/op/refstack.t - this test file contains the failing code samples
from all 68 GH tickets which were part of the RT 'stack not ref counted
issues' meta ticket. All these tests pass under PERL_RC_STACK (and are
skipped on default builds).
Added support for calling RC-aware XS subs. Note that this doesn't add
support to the XS parser for flagging XS subs as RC-aware, it merely adds
a mechanism to pp_entersub() etc to call those flagged XS functions
without wrapping them.
After merging, my pain will become everyone else's pain: any new pp()
functions you write (I'm looking at you, Paul!) should be in the new
style, using rpp_push_1() rather than PUSHs() etc.
Other than that, in theory blead shouldn't be much affected on default
builds - just a few edge cases around the handling of @db::args etc have
been tweaked.
My next major piece of work on this project will be to unwrap many of the
lightweight pp() functions, such as pp_const(). This should claw back a
lot of the performance drop.
I propose that we should add an additional GH build check thinggy
configuration (i.e. all the things that are run when you push or update a
PR):
-Duseithreads -DDEBUGGING -Accflags='-DPERL_RC_STACK'
to ensure it doesn't bitrot. I don't know who is responsible for updating
such things.
It's easy, patch the workflow file in .github. you should do it yourself
imo.
Yves
… |
|
@iabyn if you think this is stable enough, it may be worth inviting @dur-randir to throw some fuzzing at it either before or after it is merged. |
|
Now merged into blead. |
To ensure the refcounted stack build doesn't bitrot until it's the default. Updates Perl#20858
second attempt. Not for merge yet.