Skip to content

Perl_hv_common - don't call share_hek_flags when (keysv_hek != NULL)#24149

Open
richardleach wants to merge 1 commit intoPerl:bleadfrom
richardleach:keysv_hek4insert
Open

Perl_hv_common - don't call share_hek_flags when (keysv_hek != NULL)#24149
richardleach wants to merge 1 commit intoPerl:bleadfrom
richardleach:keysv_hek4insert

Conversation

@richardleach
Copy link
Contributor

When creating a new HE for insertion into a hash that supports shared keys, Perl_hv_common always achieves this via S_share_hek_flags.

S_share_hek_flags searches the shared string table - PL_strtab - for a matching entry and inserts one if needs be, returning the relevant HEK*.

However, if Perl_hv_common's keysv is a "HEK in disguise" to begin with, keysv_hek already contains the resulting HEK* - and we are able to bump the refcount on it. So this commit checks for that case and branches accordingly.

gcov counts from a perl build and test_harness run suggest that having a valid keysv_hek is already very common.

129097943:  996:    if (LIKELY(HvSHAREKEYS(hv))) {
129097941:  997:        entry = new_HE();
129097941:  998:        if (keysv_hek) {
 50255991:  999:            HeKEY_hek(entry) = keysv_hek;
 50255991: 1000:            share_hek_hek(keysv_hek);
        -: 1001:        } else {
 78841950: 1002:            HeKEY_hek(entry) = share_hek_flags(key, klen, hash, flags);
        -: 1003:        }
        -: 1004:    }

If greater use is made of PL_strtab in the future, such as for OP_CONST SVs, the keysv_hek is likely to become correspondingly hotter.

(If someone points out a compelling reason why this isn't already done, I'll add comments to that effect instead.)


  • I haven't benchmarked yet to see if this change is likely to be noticeable. It may warrant a perldelta entry.

@richardleach
Copy link
Contributor Author

A rough comparison:

my %h1 = map { $_ => 0} qw(one two three four five);
for (1 .. 100_000_000) {
    my %h2 = %h1;
}

blead:

         26,730.35 msec task-clock                       #    1.000 CPUs utilized             
               127      context-switches                 #    4.751 /sec                      
                 0      cpu-migrations                   #    0.000 /sec                      
               197      page-faults                      #    7.370 /sec                      
   124,235,900,859      cycles                           #    4.648 GHz                       
     3,323,830,317      stalled-cycles-frontend          #    2.68% frontend cycles idle      
   503,542,464,696      instructions                     #    4.05  insn per cycle            
   106,009,840,217      branches                         #    3.966 G/sec                     
       101,478,688      branch-misses                    #    0.10% of all branches  

patched:

         25,003.63 msec task-clock                       #    1.000 CPUs utilized             
               119      context-switches                 #    4.759 /sec                      
                 0      cpu-migrations                   #    0.000 /sec                      
               194      page-faults                      #    7.759 /sec                      
   116,791,723,673      cycles                           #    4.671 GHz                       
     3,307,581,712      stalled-cycles-frontend          #    2.83% frontend cycles idle      
   477,589,786,476      instructions                     #    4.09  insn per cycle            
   101,859,216,594      branches                         #    4.074 G/sec                     
        51,308,472      branch-misses                    #    0.05% of all branches

Worth doing, but I'm not sure it warrants a perldelta entry.

@jkeenan
Copy link
Contributor

jkeenan commented Feb 4, 2026

Need to get make test_porting working again for this p.r.

@mauke
Copy link
Contributor

mauke commented Feb 4, 2026

I believe the porting tests are only failing because there are some tags missing from this fork. The tests would probably succeed in the main repository.

When creating a new `HE` for insertion into a hash that supports shared
keys, `Perl_hv_common` always achieves this via `S_share_hek_flags`.

`S_share_hek_flags` searches the shared string table - `PL_strtab` -
for a matching entry and inserts one if needs be, returning the
relevant `HEK*`.

However, if `Perl_hv_common`'s `keysv` is a "HEK in disguise" to begin
with, `keysv_hek` _already_ contains the resulting `HEK*` - and we
are able to bump the refcount on it. So this commit checks for that
case and branches accordingly.

gcov counts from a perl build and test_harness run suggest that having
a valid `keysv_hek` is already very common.

```
129097943:  996:    if (LIKELY(HvSHAREKEYS(hv))) {
129097941:  997:        entry = new_HE();
129097941:  998:        if (keysv_hek) {
 50255991:  999:            HeKEY_hek(entry) = keysv_hek;
 50255991: 1000:            share_hek_hek(keysv_hek);
        -: 1001:        } else {
 78841950: 1002:            HeKEY_hek(entry) = share_hek_flags(key, klen, hash, flags);
        -: 1003:        }
        -: 1004:    }
```

If greater use is made of `PL_strtab` in the future, such as for
OP_CONST SVs, the `keysv_hek` is likely to become correspondingly
hotter.
@richardleach
Copy link
Contributor Author

I've rebased and pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants