Clarify why hv_common() tries to clear placeholders before calling hsplit().

Which makes me realise that if we clear placeholders, we may be able to
avoid the need to split at all.

(In fact, as hv_common() only adds one key to the hash, under the current
definition of DO_HSPLIT() which only considers total number of keys,
clearing any placeholder is going to be enough to drop the total number of
keys, and so no longer trigger the split. But we'll leave the code making a
second check, to avoid a tight coupling with the internals of DO_HSPLIT().)

1 files changed, 15 insertions, 9 deletions

diff --git a/hv.c b/hv.c
index af722f6802..33387dc2de 100644
--- a/hv.c
+++ b/hv.c
@@ -798,17 +798,23 @@ Perl_hv_common(pTHX_ HV *hv, SV *keysv, const char *key, STRLEN klen,
     if ( DO_HSPLIT(xhv) ) {
         const STRLEN oldsize = xhv->xhv_max + 1;
 
-        /* This logic was in S_hsplit, but as the shared string table can't
-           contain placeholders, and we are the only other caller of S_hsplit,
-           it could only trigger from this callsite. So move it here.  */
-        if (HvPLACEHOLDERS_get(hv) && !SvREADONLY(hv)) {
-            /* Can make this clear any placeholders first for non-restricted
-               hashes, even though Storable rebuilds restricted hashes by
+        if (HvPLACEHOLDERS_get(hv) /* hash has placeholders  */
+            && !SvREADONLY(hv) /* but is not a restricted hash */) {
+            /* If this hash previously was a "restricted hash" and had
+               placeholders, but the "restricted" flag has been turned off,
+               then the placeholders no longer serve any useful purpose.
+               However, they have the downsides of taking up RAM, and adding
+               extra steps when finding used values. It's safe to clear them
+               at this point, even though Storable rebuilds restricted hashes by
                putting in all the placeholders (first) before turning on the
-               readonly flag, because Storable always pre-splits the hash.  */
+               readonly flag, because Storable always pre-splits the hash.
+               If we're lucky, then we may clear sufficient placeholders to
+               avoid needing to split the hash at all.  */
             hv_clear_placeholders(hv);
-        }
-        hsplit(hv, oldsize, oldsize * 2);
+            if (DO_HSPLIT(xhv))
+                hsplit(hv, oldsize, oldsize * 2);
+        } else
+            hsplit(hv, oldsize, oldsize * 2);
     }
 
     if (return_svp) {