Efficient HV fetches without recomputing the key hash

The HV API is a bit of a mess of functions. There's basically two ways
you can look up an item out of an HV, depending on how you have your
key:

   SV **  hv_fetch (HV *hv, const char *key, I32 klen, I32 lval);

   HE *  hv_fetch_ent(HV *hv, SV *keysv, I32 lval, U32 hash);

If you have your key as a plain char*+length pair, you can call
`hv_fetch`. If you have your key as a full SV already, you can call
`hv_fetch_ent`. The latter function allows you to pass in a precomputed
hash value; the former does not. That means if you call `hv_fetch` it
has to recalculate the hash value. That's not very efficient, if that
is already known.

When might it be known? Well,... maybe you already have it from an HE*
out of a different hash. Consider the following code chunk, which is
doing the C version of:

  $dst_hv{$_} += $src_hv{$_} for keys %src_hv;

We're going to call sv_inc() on each (possibly-new) element in dst_hv,
which we look up using each of the keys and values taken from src_hv:

  hv_iterinit(src_hv);

  HE *he;
  while((he = hv_iternext(src_hv)) {
    SV *src_sv = HeVAL(he);

    /* it's a shame I can't pass in HeHASH(he) here */
    SV **dst_svp = hv_fetch(dst_hv, HeKEY(he), HeKLEN(he), 1);

    sv_inc(*dst_svp, SvIV(src_sv));
  }

Here `hv_fetch()` needs to recalculate the hash values every time. If I
want to avoid that, I have to call `hv_fetch_ent` instead, and present
it with the key in an SV somehow. Calling `HeSVKEY_force()` would give
me that, but that's going to be just as inefficient as it has to copy
the string buffer.

I *believe* on a quick read through hv.c, that `hv_fetch` isn't going
to store or modify the keysv that is passed into it, so I might be safe
to do subtle "just reuse the string buffer pointer" trickery with it;
via something like:

  SV *keysv = newSV(0);
  SvPOK_on(keysv);
  SAVEFREESV(keysv);

  HE *he;
  while((he = hv_iternext(src_hv)) {
    SV *src_sv = HeVAL(he);

    SvPVX(keysv) = HeKEY(he);
    SvCUR_set(keysv, HeKLEN(he));
    /* should also do something about the UTF-8 flag here but I'm
     * omitting that for clarity of this example ;) */

    HE *dst_he = hv_fetch_ent(dst_hv, keysv, 0, HeHASH(he));
    ...
  }

That looks pretty hacky and subtle though. Is this the best approach?

Alternative ideas:

  * Directly reach-around the `hv_fetch*` macros by observing that
    they are all just wrappers around hv_common(), and make myself a
    brand new wrapper, something like

      #define hv_fetch_with_hash(hv, key, klen, lval, hash)    \
        hv_common_key_len((hv), (key), (klen),                 \
          (HV_FETCH_JUST_SV | ((lval) ? HV_FETCH_LVALUE : 0)), \
          NULL, (hash))

    That's a straight-up copy of hv_fetch, just passing in (hash)
    rather than 0 for the hash value.

    Downside: not guaranteed to continue to work in newer perls

  * Add a new API function with this shape, and then call it.

      SV **hv_fetch_with_hash(HV *hv, const char *key, I32 klen,
          I32 lval, U32 hash);

    Downside: doesn't work on older perls.

Maybe we could do both - define a new wrapper, and also suggest that
folks can call the hv_common directly with it on older perls?


Also, all of the above comments can be made also about "exists" and
"delete" operations. But not "store", because even the char*+length
taking `hv_store` function *does* take a `U32 hash` parameter.

---

Overall, I think it's a bit of a mess that these functions all eat an
entire `I32 lval` parameter simply for its truth. That really ought to
have been a `U32 flags`, which could then be used to contain a bunch of
different behaviours... but eh. It's a bit late now to start fixing all
of these things...

-- 
Paul "LeoNerd" Evans

[email protected]
http://www.leonerd.org.uk/  |  https://metacpan.org/author/PEVANS