Mailing List Archive

On Sun, Feb 26, 2023 at 5:06 PM Dave Mitchell <davem@iabyn.com> wrote:

> TL;DR:
>
> The branch containing the first stage of my "make the stack ref-counted"
> work is nearly ready for merging.


I have nothing particular to contribute to the ongoing conversation except
to say "thank you" to Dave for this amazing work. It's much appreciated.

Best,
Ovid

Overall: This is some excellent stuff here. I am most looking forward
to seeing it land.

Pace-wise I agree with other comments that it might be best to save it
for a 5.39.0 rather than try to squeeze it into 5.37 at this late stage
- especially since it might have unforseen interactions with my recent
feature-class work. I know I do some slightly funky things with the
stacks in there, so it would be good not to do a "Solaris 10" [1]


[1]: Solaris 10 launched with three big new features - Zones, ZFS and
DTrace. They were all merged so late in the development process
that none of them was aware of the others. When 10 launched, zones
could not use ZFS as their root filesystem, dtrace could not cross
zone boundaries, and ZFS operations did not appear in dtrace
output. Fun times.


On Sun, 26 Feb 2023 16:06:12 +0000
Dave Mitchell <davem@iabyn.com> wrote:

> The first 16 commits in this branch are generic fixups for things I
> spotted while working on this branch, and could be committed even if
> this branch was rejected overall.

It sounds like Yves is already on the ball on that, but yes if they're
smallish tidying up the could go in first.

> which should be required reading for any core hackers. I've cut and
> pasted it at the bottom of this email for convenience.

You actually pasted it twice - once in the middle of the other ;) It's
not *quite* as scary-long as the email size makes it look.

> The whole C<dSP> thing harks back to the days before decent optimising
> compilers. It was always error-prone, e.g. if you forgot a C<PUTBACK>
> or C<SPAGAIN>. The new API always just accesses C<PL_stack_sp>
> directly. In fact the first step of upgrading a PP function is always
> to remove the C<dSP> declaration. This has the happy side effect that
> any old-style macros left in the pp function which implicitly use
> C<sp> will become compile errors. The existence of a C<dSP> somewhere
> in core is a good sign that that function still needs updating.

OMG +1 to this alone. If nothing else, getting rid of that entire dSP
/ PUTBACK / SPAGAIN dance would make me much happy. I've now lost count
the number of times I've had a hard-to-trace bug that turned out to be
a missing SPAGAIN that *usually* didn't matter, so hunting it down was
very unreliable. It would be great not to have to worry about these any
more.g

--
Paul "LeoNerd" Evans

leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS
http://www.leonerd.org.uk/ | https://www.tindie.com/stores/leonerd/

On 2/28/23 07:58, Ovid wrote:
> On Sun, Feb 26, 2023 at 5:06 PM Dave Mitchell <davem@iabyn.com> wrote:
>
> TL;DR:
>
> The branch containing the first stage of my "make the stack
> ref-counted"
> work is nearly ready for merging.
>
> I have nothing particular to contribute to the ongoing conversation
> except to say "thank you" to Dave for this amazing work. It's much
> appreciated.
>
> Best,
> Ovid

I am also very grateful.

I don’t know why this fix makes me so excited (maybe because this has
been such a long standing issue or because I need to get out more), but
it does.

Thanks Dave!

On Tue, Feb 28, 2023 at 11:03 AM Darren Duncan <darren@darrenduncan.net>
wrote:

> On 2023-02-27 2:50 a.m., demerphq wrote:
> > On Sun, 26 Feb 2023 at 17:06, Dave Mitchell wrote:
> > The branch containing the first stage of my "make the stack
> ref-counted"
> > work is nearly ready for merging. In a default build, the core is
> mostly
> > unchanged: only when built with -DPERL_RC_STACK does it behave
> > differently. It needs testing on Windows. I need to decide whether to
> > merge soon or wait until after 5.38.
> >
> > I think it has to wait till 5.38, at least as far as normal policy goes.
> I guess
> > it depends on whether we consider this a contentious code-change, but
> given its
> > scale it seems like we should hold off. (It brings me no joy to say this
> BTW, id
> > be more than happy if my understanding was incorrect.)
>
> To be completely clear, in a default build, should all known XS also
> compile and
> run properly without alterations? Or does the "mostly unchanged" exclude
> some
> things that XS may be sensitive to?
>

I would personally expect xsubpp to need updating to avoid the performance
degradation (and for PPCODE xsubs I expect they will need updating to not
have the degradation), but it should be functional without that.

Leon

On Tue, Feb 28, 2023 at 01:27:06PM +0100, Leon Timmermans wrote:
> Yeah, at this point it's probably better to introduce this early in the
> next cycle. And possibly then enable it by default (even if we might not
> yet do that for production releases) to shake all possible issues out.

We might want to leave temporarily enabling it until all the issues we
already know with CPAN have been fixed. For example, any XS module which
uses a custom OP or PP function is likely to break.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

On Wed, Mar 01, 2023 at 01:06:16AM +0100, Leon Timmermans wrote:
> I would personally expect xsubpp to need updating to avoid the performance
> degradation (and for PPCODE xsubs I expect they will need updating to not
> have the degradation), but it should be functional without that.

"Thinking about the XS API" is one of the things on my todo list, but is
relatively far down. I want to get core working at roughly old speeds
first.

--
Music lesson: a symbiotic relationship whereby a pupil's embellishments
concerning the amount of practice performed since the last lesson are
rewarded with embellishments from the teacher concerning the pupil's
progress over the corresponding period.

On Wed, 1 Mar 2023 at 13:46, Dave Mitchell <davem@iabyn.com> wrote:

> On Tue, Feb 28, 2023 at 01:27:06PM +0100, Leon Timmermans wrote:
> > Yeah, at this point it's probably better to introduce this early in the
> > next cycle. And possibly then enable it by default (even if we might not
> > yet do that for production releases) to shake all possible issues out.
>
> We might want to leave temporarily enabling it until all the issues we
> already know with CPAN have been fixed. For example, any XS module which
> uses a custom OP or PP function is likely to break.
>

For what it's worth the branch has already help shake out a bug in
Text::CSV_XS. If that precedent is not an anomaly it looks like XS modules
that do not create PP functions that are broken by the branch have a defect
already and the branch is a good way to find bugs. (Seems like a win to me)

Sereal::Encoder/Decoder do support custom opcode stuff but afaik they did
not break. Not sure why yet. Is it expected that some such code would still
be fine?

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Mar 01, 2023 at 01:53:03PM +0100, demerphq wrote:
> Sereal::Encoder/Decoder do support custom opcode stuff but afaik they did
> not break. Not sure why yet. Is it expected that some such code would still
> be fine?

Any non-core PP function (called either via a custom OP or from a standard
perl OP where the op_ppaddr has been replaced with the user's PP function)
and which hasn't been wrapped nor updated will tend to:
- leak any args passed to it;
- have any values it returns be prematurely freed later;
due to the reference-count bookkeeping not being done.

--
Fire extinguisher (n) a device for holding open fire doors.

Dave Mitchell wrote:
>The branch containing the first stage of my "make the stack ref-counted"
>work is nearly ready for merging.

I'm pleased that the effort is being made to make the stack refcounted,
and from reading the new perlguts documentation it seems to me that this
is a good way to go about it.

> There is not yet an API to allow XS functions to be
> written in an RC-aware way.

Please add such a thing before merging. I have some modules that share
the stack-manipulating guts of operations between PP functions for custom
ops and XSUBs, and since I want the PP functions to be optimally fast
I'd be unhappy with making the PP functions wrap shared RC-unaware guts.
If XSUBs couldn't be marked as RC-aware then I'd have to develop a
wrapper to let XSUB code running with a non-RC stack call RC-using
PP code. That need arises as soon as there's a Perl release on which
we expect PERL_RC_STACK builds to be supported but which doesn't support
RC-aware XSUBs.

The necessary API is only a CVf_ flag, which can be set by C code and
which pp_entersub consults to determine whether to do the split-stack
wrapping. I can set this flag when I'm constructing XSUB CVs from
manually-written C code, which is generally how I construct them when
sharing guts with PP functions. This doesn't require any support in
ExtUtils::ParseXS.

Eventually we'd want EU:PXS to have some syntax such as "RCPPCODE:"
to declare that a sub body is RC-aware. This would make it generate
appropriate prologue and epilogue code in the XSUB body function,
and make it generate boot code that sets the CVf_ flag on the XSUB.
But this facility for ordinary XS code isn't as urgent as having the
underlying API facility.

>* Any XS code which includes a custom OP with its own PP function, or
> contains a PP function intended to override a core PP function ,won't
> work unmodified;.

As things stand, this kind of code would still compile fine, and just
screw up the refcounting at runtime. It would be helpful if it were
possible to make some gratuitous API change that prevents the confusion
of old-style and new-style PP functions. Ideally this would be a type
change that makes the function pointers incompatible at the C level.
The obvious way to do that would be to add a new parameter that is never
used, but I don't see how to avoid that imposing overhead in setting a
register value for that parameter.

-zefram

On Sun, Mar 19, 2023 at 05:08:59PM +0000, Zefram via perl5-porters wrote:
> > There is not yet an API to allow XS functions to be
> > written in an RC-aware way.
>
> Please add such a thing before merging.

I'll certainly add a new CV flag sometime soon to indicate this.
If I don't get time to do it right away, it may come a bit after merging,
since I'll want to merge this branch ASAP after 5.39 is open, due the
increasing difficulty of keeping it synced with blead.

> Eventually we'd want EU:PXS to have some syntax such as "RCPPCODE:"

Yeah. I haven't even begun to consider what the new XS API should look
like yet: I want to get the basic code working and merged first.

> >* Any XS code which includes a custom OP with its own PP function, or
> > contains a PP function intended to override a core PP function ,won't
> > work unmodified;.
>
> As things stand, this kind of code would still compile fine, and just
> screw up the refcounting at runtime.

On debugging builds with DEBUG_LEAKING_SCALARS enabled, PUSHs() etc assert
that they are operating on a !AvREAL() stack (and vice-versa with
rpp_push_1() etc). I'm thinking of making those asserts present on all
PERL_RC_STACK+DEBUGGING builds, without DEBUG_LEAKING_SCALARS being
needed. (Initially I was worried about slowing down debugging builds too
much, but that doesn't seem to have happened).

That should be enough to catch such issues?`

--
"I used to be with it, but then they changed what ‘it’ was, and now what
I’m with isn’t it. And what’s ‘it’ seems weird and scary to me."
-- Grandpa Simpson
(It will happen to you too.)

Dave Mitchell wrote:
>If I don't get time to do it right away, it may come a bit after merging,

That sounds fine.

>That should be enough to catch such issues?`

That'll certainly mean that affected CPAN distros get a bunch of failure
reports. That should suffice.

-zefram