In perl.git, the branch smueller/hash_vtable has been updated <http://perl5.git.perl.org/perl.git/commitdiff/5d9415065b621805298a78b0ec7862257128fcc5?hp=0b5522d0e0c84d15711e650e6e4e92109e23a2af>
- Log ----------------------------------------------------------------- commit 5d9415065b621805298a78b0ec7862257128fcc5 Author: Steffen Mueller <[email protected]> Date: Fri Feb 3 09:21:59 2017 +0100 Hash vtables: Sigh, vtables for key count access This is ugly. Ugly. Ugly. Ugly. This adds vtables for HvTOTALKEYS and friends. First off, the distinction of HvTOTALKEYS and HvUSEDKEYS is a bit dodgy/special-purpose for the placeholder logic. Which is annoying in that we even have to deal with that corner case. Furthermore, everyone and their kitchen sink seems to assume that HvTOTALKEYS is an lvalue. At which point having that as a macro abstraction is a point almost defeated. A mythical "HvTOTALKEYS_set" wouldn't make sense either since even writing to such a thing would make little to no sense for other hash implementations. Maybe the right thing to do is to have a HvTOTALKEYS_set and a vtable entry that just ignores it by default? Sigh! M hv.c M hv.h M hv_vtbl.c M hv_vtbl.h M perl.c commit 0d5a29a4ee7792c6709d34c5ccb2114104a04b93 Author: Steffen Mueller <[email protected]> Date: Fri Feb 3 09:06:41 2017 +0100 HvTOTALKEYS() takes a HV* as argument Incidentally, it currently works on SV *'s as well because there's an explicit cast after an SvANY. Let's not rely on that. This commit also removes a pointless const in a cast. Again. It takes an HV * as argument. Let's only change that if we have a strong reason to. M dump.c M hv.c M hv.h M pp_hot.c ----------------------------------------------------------------------- Summary of changes: dump.c | 2 +- hv.c | 30 +++++++++++++++++++++++++----- hv.h | 10 +++++++--- hv_vtbl.c | 17 ++++++++++++++++- hv_vtbl.h | 11 ++++++++++- perl.c | 9 ++++++++- pp_hot.c | 2 +- 7 files changed, 68 insertions(+), 13 deletions(-) diff --git a/dump.c b/dump.c index 9edc8bf7db..89779b6998 100644 --- a/dump.c +++ b/dump.c @@ -1886,7 +1886,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo (UV)aux->xhv_aux_flags); } Perl_dump_indent(aTHX_ level, file, " ARRAY = 0x%" UVxf, PTR2UV(HvARRAY(sv))); - usedkeys = HvUSEDKEYS(sv); + usedkeys = HvUSEDKEYS(MUTABLE_HV(sv)); if (HvARRAY(sv) && usedkeys) { /* Show distribution of HEs in the ARRAY */ int freq[200]; diff --git a/hv.c b/hv.c index f65c93e83c..b22a32c98a 100644 --- a/hv.c +++ b/hv.c @@ -1066,7 +1066,7 @@ Perl_hv_bucket_ratio(pTHX_ HV *hv) } sv = sv_newmortal(); - if (HvUSEDKEYS((const HV *)hv)) + if (HvUSEDKEYS((HV *)hv)) Perl_sv_setpvf(aTHX_ sv, "%ld/%ld", (long)HvFILL(hv), (long)HvMAX(hv) + 1); else @@ -1621,7 +1621,12 @@ Perl_newHVhv(pTHX_ HV *ohv) } HvMAX(hv) = hv_max; - HvTOTALKEYS(hv) = HvTOTALKEYS(ohv); + /* Used to do "HvTOTALKEYS(hv) = HvTOTALKEYS(ohv);" + * here, but that assumes HvTOTALKEYS is an lvalue. + * That's no longer true due to vtable-hashes, so since + * this code-path is for assigning to a traditional + * hash only, we unroll HvTOTALKEYS on the left hand side. */ + ((XPVHV *)SvANY(hv))->xhv_keys = HvTOTALKEYS(ohv); HvARRAY(hv) = ents; } /* not magical */ else { @@ -1893,7 +1898,14 @@ S_clear_placeholders(pTHX_ HV *hv, U32 items) if (--items == 0) { /* Finished. */ I32 placeholders = HvPLACEHOLDERS_get(hv); - HvTOTALKEYS(hv) -= (IV)placeholders; + /* We used to directly modify HvTOTALKEYS here, but + * with vtable hashes, that's no longer an lvalue, + * so have to unroll it. */ + /* FIXME consider what happens with vtable hash on + * left hand side here. Eventually, locked + * hashes will hopefully live INSIDE a + * vtable? */ + ((XPVHV *)SvANY(hv))->xhv_keys -= (IV)placeholders; /* HvUSEDKEYS expanded */ if ((HvTOTALKEYS(hv) - placeholders) == 0) HvHASKFLAGS_off(hv); @@ -3321,7 +3333,12 @@ Perl_refcounted_he_chain_2hv(pTHX_ const struct refcounted_he *chain, U32 flags) HeNEXT(entry) = *oentry; *oentry = entry; - HvTOTALKEYS(hv)++; + /* FIXME HvTOTALKEYS(hv)++ assumed that HvTOTALKEYS + * is an lvalue. But that's not a thing with + * hash vtables. So unroll. :( + * This should be safe (enough) for now since + * we create the regular HV in this function. */ + (((XPVHV *)SvANY(hv))->xhv_keys )++; next_please: chain = chain->refcounted_he_next; @@ -3329,7 +3346,10 @@ Perl_refcounted_he_chain_2hv(pTHX_ const struct refcounted_he *chain, U32 flags) if (placeholders) { clear_placeholders(hv, placeholders); - HvTOTALKEYS(hv) -= placeholders; + /* FIXME see comment above. Original code: + * HvTOTALKEYS(hv) -= placeholders; + */ + (((XPVHV *)SvANY(hv))->xhv_keys) -= placeholders; } /* We could check in the loop to see if we encounter any keys with key diff --git a/hv.h b/hv.h index d66d14adb3..f2790b20ff 100644 --- a/hv.h +++ b/hv.h @@ -329,7 +329,7 @@ C<SV*>. ((SvOOK(hv) && HvAUX(hv)->xhv_name_u.xhvnameu_name && HvAUX(hv)->xhv_name_count != -1) \ ? HEK_UTF8(HvENAME_HEK_NN(hv)) : 0) -/* the number of keys (including any placeholders) */ +/* the number of keys (including any placeholders) - NOT PART OF THE API */ #define XHvTOTALKEYS(xhv) ((xhv)->xhv_keys) /* @@ -338,8 +338,12 @@ C<SV*>. * (keys, excluding placeholders) and HvTOTALKEYS (including placeholders) */ #define HvKEYS(hv) HvUSEDKEYS(hv) -#define HvUSEDKEYS(hv) (HvTOTALKEYS(hv) - HvPLACEHOLDERS_get(hv)) -#define HvTOTALKEYS(hv) XHvTOTALKEYS((XPVHV*) SvANY(hv)) +#define HvUSEDKEYS(hv) (HvHASVTBL(hv) ? \ + HvVTBL(hv)->hvt_usedkeys(aTHX_ hv) : \ + (HvTOTALKEYS(hv) - HvPLACEHOLDERS_get(hv))) +#define HvTOTALKEYS(hv) (HvHASVTBL(hv) ? \ + HvVTBL(hv)->hvt_totalkeys(aTHX_ hv) : \ + XHvTOTALKEYS((XPVHV*) SvANY(hv))) #define HvPLACEHOLDERS(hv) (*Perl_hv_placeholders_p(aTHX_ MUTABLE_HV(hv))) #define HvPLACEHOLDERS_get(hv) (SvMAGIC(hv) ? Perl_hv_placeholders_get(aTHX_ (const HV *)hv) : 0) #define HvPLACEHOLDERS_set(hv,p) Perl_hv_placeholders_set(aTHX_ MUTABLE_HV(hv), p) diff --git a/hv_vtbl.c b/hv_vtbl.c index 5a98ffb1f4..f2d6706b51 100644 --- a/hv_vtbl.c +++ b/hv_vtbl.c @@ -242,6 +242,19 @@ S_hv_mock_std_vtable_exists(pTHX_ HV *hv, SV *keysv, const char *key, return retval; } +STATIC STRLEN +S_hv_mock_std_vtable_totalkeys(pTHX_ HV *hv) +{ + return ((XPVHV *)SvANY(hv))->xhv_keys; +} + +STATIC STRLEN +S_hv_mock_std_vtable_usedkeys(pTHX_ HV *hv) +{ + /* total keys minus placeholders */ + return ((XPVHV *)SvANY(hv))->xhv_keys - HvPLACEHOLDERS_get(hv); +} + HV_VTBL PL_mock_std_vtable = { S_hv_mock_std_vtable_init, S_hv_mock_std_vtable_destroy, @@ -252,7 +265,9 @@ HV_VTBL PL_mock_std_vtable = { S_hv_mock_std_vtable_exists, S_hv_mock_std_vtable_delete, S_hv_mock_std_vtable_clear, - S_hv_mock_std_vtable_undef + S_hv_mock_std_vtable_undef, + S_hv_mock_std_vtable_totalkeys, + S_hv_mock_std_vtable_usedkeys }; /* diff --git a/hv_vtbl.h b/hv_vtbl.h index 635f218e3b..fbcd509f17 100644 --- a/hv_vtbl.h +++ b/hv_vtbl.h @@ -38,6 +38,16 @@ struct hv_vtbl { * some internal hack. Needs more thinking! */ void (*hvt_undef)(pTHX_ HV *hv, U32 flags); + /* Returns the total number of keys (including placeholders) */ + /* FIXME there's code that uses HvTOTALKEYS in lvalue context, eg. for hash cloning. + * CPAN doesn't really have anything that does that legitimately, but it exists + * in core. + * The issue is that depending on the hash implementation, such a thing is completely + * nonsensical, so simply exposing some potential HvTOTALKEYS_set API wouldn't make sense! */ + STRLEN (*hvt_totalkeys)(pTHX_ HV *hv); + /* Returns the number of keys used (ie. not including placeholders) */ + STRLEN (*hvt_usedkeys)(pTHX_ HV *hv); + /* TODO also wrap all the iteration primitives! */ /* TODO research what other primitives are missing! */ /* TODO what about all the hash introspection macros? HvTOTALKEYS? etc etc? */ @@ -49,7 +59,6 @@ struct hv_vtbl { /* TODO What about the "hash name" related stuff (for stashes?)? */ /* TODO what about hv_magic? */ /* TODO what about placeholders? */ - /* TODO hv_assert? Bother? */ /* TODO once 'everything' is wrapped, one way to test is to use the mock vtbl implementation * to 'move' or 'rotate' all struct members in some well-defined way that can be undone diff --git a/perl.c b/perl.c index 09eb2f4f62..0265a1233b 100644 --- a/perl.c +++ b/perl.c @@ -1238,7 +1238,14 @@ perl_destruct(pTHXx) Safefree(array); HvARRAY(PL_strtab) = 0; - HvTOTALKEYS(PL_strtab) = 0; + /* FIXME Old code was: + * HvTOTALKEYS(PL_strtab) = 0; + * But that assumes HvTOTALKEYS is an lvalue, which is + * incorrect for vtable hashes. So we have to unroll + * that for now. PL_strtab is not a vtable hash as of + * this point in time, so that's safe (if awful). */ + (((XPVHV *)SvANY(hv))->xhv_keys) = 0; + } SvREFCNT_dec(PL_strtab); diff --git a/pp_hot.c b/pp_hot.c index aeaecfc3df..a3ee2a7390 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -1039,7 +1039,7 @@ PP(pp_rv2av) || ( PL_op->op_private & OPpMAYBE_TRUEBOOL && block_gimme() == G_VOID )) && (!SvRMAGICAL(sv) || !mg_find(sv, PERL_MAGIC_tied))) - SETs(HvUSEDKEYS(sv) ? &PL_sv_yes : &PL_sv_no); + SETs(HvUSEDKEYS(MUTABLE_HV(sv)) ? &PL_sv_yes : &PL_sv_no); else if (gimme == G_SCALAR) { dTARG; TARG = Perl_hv_scalar(aTHX_ MUTABLE_HV(sv)); -- Perl5 Master Repository
