class.c: clean up any state if we don't finish the class #22374

tonycoz · 2024-07-04T00:22:46Z

class.c

jkeenan · 2024-08-26T13:50:11Z

@leonerd, could you please review this p.r. from @tonycoz? Thanks.

bulk88 · 2024-10-17T19:32:36Z

class.c

+    struct xpvhv_aux *aux = HvAUX(stash);
+
+    SvREFCNT_dec(aux->xhv_class_superclass);
+    aux->xhv_class_superclass = NULL;


I suggest lifting the pointers to a C autos, and setting struct field to NULL, then call SvREFCNT_dec. Analyse if SvREFCNT_dec_NN is appropriate.

xhv_class_superclass can be NULL if the class has no base class defined.

I don't think there's any value re-ordering here, since we only write to xhv_class_superclass after the REFCNT_dec.

I think it's unlikely the cache line will expire - this is a reference to an existing class, so it's just going to reduce the reference count and test it.

bulk88 · 2025-01-13T22:18:34Z

class.c

+            if (SvTYPE(entry) == SVt_PVGV
+                && (cv = GvCV((GV*)entry))
+                && (CvIsMETHOD(cv) || memEQs(kpv, klen, "new"))) {
+                SvREFCNT_dec(cv);


Add _NN(), cv already proved to be derefable

bulk88 · 2025-01-13T22:21:25Z

class.c

+            }
+            else if (SvTYPE(entry) == SVt_PVCV
+                     && (CvIsMETHOD((CV*)entry) || memEQs(kpv, klen, "new"))) {
+                (void)hv_delete(stash, kpv, HeUTF8(he) ? -(I32)klen : (I32)klen,


Pass the HEK* maybe, or U32 hash. hv_delete_ent() maybe??? XS api is not fully there for exotic with HEK with hash, not fetch, HV operations.

hv_delete_ent() would require making a SV, this would only be efficient for the (I think) atypical case of the key being an SVKEY.

bulk88 · 2025-01-13T22:23:10Z

class.c

+
+    /* remove any ISA entries */
+    SV *isaname = newSVpvf("%" HEKf "::ISA", HvNAME_HEK(stash));
+    sv_2mortal(isaname);


sv_2mortal(isaname); to isaname = sv_2mortal(isaname);, use the free retval register.

bulk88 · 2025-01-13T22:29:18Z

class.c

-                newSVOP(OP_CONST, 0, (SV *)superaux->xhv_class_initfields_cv),
-                NULL);
+        U32 fieldix = PadnameFIELDINFO(pn)->fieldix;
+        (void)hv_store_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), newSVuv(padix), 0);


is there a better way of doing this other than a trip through SvPV(3args)'s S_uiv_2buf(), or worse going to libc sprintf with dozens of lock aq/releases, and maybe some kernel calls to the FS. I would just have 4 byte / 8 byte UVs stored straight as raw unprintable binary with SvPOK_on(). Hash algo/perl just sees char strings, vs turning them into longer less dense ascii bytes.

This code was only re-indented

bulk88 · 2025-01-13T22:34:22Z

class.c

+        HE *he;
+        if((he = hv_fetch_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), 0, 0)) &&
+           SvOK(HeVAL(he))) {
+            fieldop->op_targ = SvUV(HeVAL(he));


factor out HeVAL(he), get the SV*. its too many layers deep of derefs in code, and probably does optimize, but its 3 or 4 derefs now from the C auto. if accidents happen with multi eval, they will be smaller.

This code was only re-indented

bulk88 · 2025-01-13T22:35:43Z

class.c

+        fieldop->op_private = op_priv;
+
+        HE *he;
+        if((he = hv_fetch_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), 0, 0)) &&


idk if this is a loop, but if it is, y not reuse the sv_2mortal(newSVuv()); ?

It is a loop, but this code was only re-indented.

jkeenan · 2025-03-19T20:56:28Z

@tonycoz do you have a feeling for whether or not we're going to be able to merge this pull request into blead in this dev cycle or not? If not, then I recommend we label it 'defer-next-dev'.

tonycoz · 2025-03-19T23:34:22Z

It's not going anywhere until @leonerd looks at it

ap · 2025-04-24T16:43:50Z

@leonerd Ping?

leonerd · 2025-04-24T17:06:08Z

t/class/gh22169.t

+    field $x = "First";
+    field $w :reader;
+    ADJUST {
+        fail("ADJUST from failed class definition called");


Likely wants to be ::fail()

Fixed, thanks.

leonerd

Code addition looks fine.
I haven't read in detail the large changes to Perl_class_seal_stash() but I presume this is mostly just a re-indentation exercise with no change besides invoking the new cleanup function in the failure case.

I have read the new .t file about four times and I imagine it's supposed to invoke a compiletime error in the eval STR but I can't see what the error is supposed to be. Could it be made clearer there?

tonycoz · 2025-04-27T23:18:12Z

I haven't read in detail the large changes to Perl_class_seal_stash() but I presume this is mostly just a re-indentation exercise with no change besides invoking the new cleanup function in the failure case.

Yes, Except for some re-wrapping it's just indentation, the github diff viewer has an option to ignore whitespace which is handy for changes like this:

tonycoz · 2025-04-27T23:22:04Z

I have read the new .t file about four times and I imagine it's supposed to invoke a compiletime error in the eval STR but I can't see what the error is supposed to be. Could it be made clearer there?

There's no closing }, I'll add a comment.

Fixes Perl#22169

tonycoz · 2025-04-28T00:22:08Z

Squashed the two commits since it didn't seem worth messing with adding to the first commit and dealing with the conflicts in moving the code to the new function.

leonerd

Ah yes, that test makes sense now. LGTM

tonycoz requested a review from leonerd July 4, 2024 00:22

mauke reviewed Jul 4, 2024

View reviewed changes

class.c Outdated Show resolved Hide resolved

tonycoz force-pushed the 22169-class-cleanup branch from 977ed47 to 177bd8b Compare July 8, 2024 00:29

jkeenan added the class Issues related to 'class' keyword or __CLASS__ label Jul 9, 2024

bulk88 reviewed Oct 17, 2024

View reviewed changes

tonycoz mentioned this pull request Nov 28, 2024

deparse doesn't handle classes #22777

Open

bulk88 reviewed Jan 13, 2025

View reviewed changes

tonycoz added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Mar 19, 2025

leonerd reviewed Apr 24, 2025

View reviewed changes

leonerd requested changes Apr 24, 2025

View reviewed changes

class.c: clean up any state if we don't finish the class

32bd2da

Fixes Perl#22169

tonycoz force-pushed the 22169-class-cleanup branch from c4e20ea to 32bd2da Compare April 28, 2025 00:18

leonerd approved these changes Apr 28, 2025

View reviewed changes

class.c: clean up any state if we don't finish the class #22374

Are you sure you want to change the base?

class.c: clean up any state if we don't finish the class #22374

Uh oh!

Conversation

tonycoz commented Jul 4, 2024

Uh oh!

Uh oh!

jkeenan commented Aug 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeenan commented Mar 19, 2025

Uh oh!

tonycoz commented Mar 19, 2025

Uh oh!

ap commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonerd left a comment

Choose a reason for hiding this comment

Uh oh!

tonycoz commented Apr 27, 2025

Uh oh!

tonycoz commented Apr 27, 2025

Uh oh!

tonycoz commented Apr 28, 2025

Uh oh!

leonerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!