gh-158816: Skip useless hash table probe when deleting from split dicts

[hetaozdh]

Summary

delitem_common() currently calls lookdict_index() before checking whether the dictionary is combined or split.

For split dictionaries, the returned hash-table slot is not used. Therefore, the probe performed by lookdict_index() is redundant for split-table deletions.

I propose moving lookdict_index() into the combined-table branch avoiding a full probe sequence for every deletion from a split dictionary.

Benchmark

Measured with a free-threaded build, -Og, without PGO. The benchmark deletes keys from materialized instance dictionaries; medians over 12 alternating runs:

                    before   after    change
del d[k]             71.2     69.2 ns/op   -2.8%
d.pop(k)              98.9     96.1 ns/op   -2.8%
colliding hashes      78.0     74.4 ns/op   -4.6%

Combined-table deletions are unchanged.

Clang also does not sink the lookdict_index() call into the branch at -O3 -DNDEBUG, so the improvement is not specific to the local -Og build.

Validation

The following test modules pass:

• test_dict
• test_dictviews
• test_dictcomps

• Issue: Skip useless hash table probe when deleting from split dicts #158816

[hetaozdh]

Benchmark code:

#!/usr/bin/env python3
"""A/B benchmark: deleting from split vs combined dicts.

Compares two CPython builds (baseline and patched) on:

  * split_del         ``del d[k]`` on split tables
  * split_pop         ``d.pop(k)`` on split tables
  * combined_del      ``del d[k]`` on combined tables (control, must not change)
  * split_del_collide ``del d[k]`` on split tables whose keys all collide

Split dicts are materialized instance ``__dict__`` objects (the object uses
inline values, then ``vars(obj)`` turns them into a split dict).  The script
verifies via ``PyDictObject.ma_values`` that the dicts really are split / not
split; the field offset is obtained by compiling ``--offset-src``/offsets.c
against the same headers as the interpreter.  If that is unavailable the
verification is skipped and reported as such.

Only the deletions are timed; refilling the dicts happens outside the timed
region.  Every run reports the best of ROUNDS rounds; the driver runs the two
interpreters alternately and prints min/median per case.

Usage (from the CPython build tree, both binaries in that tree):

    PYTHONHASHSEED=0 python3 bench_dict_del.py \
        --baseline ./python-baseline.exe --patched ./python.exe --runs 12
"""

import argparse
import ctypes
import json
import os
import statistics
import subprocess
import sys
import tempfile
import time

NAMES = ["alpha", "beta", "gamma", "delta", "epsilon", "zeta", "eta", "theta"]
NCLASSES = 200
NPER = 20
ROUNDS = 120

OFFSETS_C = r"""
#include "Python.h"
#include <stdio.h>
#include <stddef.h>
int main(void) {
    printf("%zu\n", offsetof(PyDictObject, ma_values));
    return 0;
}
"""


def colliding_names(count):
    """Attribute names that all hash into the same slot of a 64-entry table."""
    out, i = [], 0
    while len(out) < count:
        nm = "c%d" % i
        i += 1
        if hash(nm) & 63 == 0:
            out.append(nm)
    return out


def make_split_dicts(names):
    dicts = []
    for _ in range(NCLASSES):
        cls = type("C", (), {})
        for _ in range(NPER):
            obj = cls()
            for nm in names:
                setattr(obj, nm, 1)
            dicts.append(vars(obj))
    return dicts


def timed_rounds(dicts, names, op, rounds):
    best = None
    samples = []
    for _ in range(rounds):
        t0 = time.perf_counter_ns()
        if op == "del":
            for d in dicts:
                for nm in names:
                    del d[nm]
        else:
            for d in dicts:
                for nm in names:
                    d.pop(nm)
        t1 = time.perf_counter_ns()
        for d in dicts:  # refill, not timed
            for nm in names:
                d[nm] = 1
        dt = t1 - t0
        samples.append(dt)
        if best is None or dt < best:
            best = dt
    samples.sort()
    return best, samples[len(samples) // 2]


def measure(offset):
    import gc
    gc.disable()

    split = make_split_dicts(NAMES)
    combined = [dict(d) for d in split]
    coll = colliding_names(len(NAMES))
    split_coll = make_split_dicts(coll)

    info = {
        "split_sizeof": split[0].__sizeof__(),
        "combined_sizeof": combined[0].__sizeof__(),
        "ndicts": len(split),
        "rounds": ROUNDS,
        "verified": offset is not None,
    }
    if offset is not None:
        is_split = lambda d: bool(ctypes.c_void_p.from_address(id(d) + offset).value)
        info["split_dicts_split"] = sum(is_split(d) for d in split)
        info["combined_dicts_split"] = sum(is_split(d) for d in combined)
        if info["split_dicts_split"] != len(split):
            raise SystemExit("ERROR: instance dicts are not split: %r" % info)
        if info["combined_dicts_split"] != 0:
            raise SystemExit("ERROR: control dicts are not combined: %r" % info)

    cases = [
        ("split_del", split, NAMES, "del"),
        ("split_pop", split, NAMES, "pop"),
        ("combined_del", combined, NAMES, "del"),
        ("split_del_collide", split_coll, coll, "del"),
    ]
    results = []
    for name, dicts, names, op in cases:
        best, p50 = timed_rounds(dicts, names, op, ROUNDS)
        ops = len(dicts) * len(names)
        results.append({
            "case": name,
            "nops_per_round": ops,
            "best_ns_per_op": best / ops,
            "p50_ns_per_op": p50 / ops,
        })
    return {"info": info, "results": results}


def compile_offsets(src):
    """Compile offsets.c against the interpreter's headers; None on failure."""
    if src is None:
        return None
    cc = os.environ.get("CC", "cc")
    with tempfile.TemporaryDirectory() as tmp:
        cfile = os.path.join(tmp, "offsets.c")
        exe = os.path.join(tmp, "offsets")
        with open(cfile, "w") as f:
            f.write(OFFSETS_C)
        cmd = [cc, "-I" + src, "-I" + os.path.join(src, "Include"),
               "-I" + os.path.join(src, "Include", "internal"),
               "-DPy_BUILD_CORE", "-o", exe, cfile]
        try:
            subprocess.run(cmd, check=True, capture_output=True)
            return int(subprocess.check_output([exe]).strip())
        except (OSError, subprocess.SubprocessError, ValueError):
            return None


def run_worker(offset):
    print(json.dumps(measure(offset)))


def run_driver(args):
    offset = None
    if args.no_verify:
        print("split verification: disabled (--no-verify)", file=sys.stderr)
    else:
        offset = compile_offsets(args.offset_src)
        if offset is None:
            print("split verification: could not compile offsets.c; skipped",
                  file=sys.stderr)

    this = os.path.abspath(__file__)
    runs = {"baseline": [], "patched": []}
    for i in range(args.runs):
        order = [("baseline", args.baseline), ("patched", args.patched)]
        if i % 2:
            order.reverse()
        for tag, binary in order:
            binary = os.path.abspath(binary)
            cmd = [binary, this, "--worker"]
            if offset is not None:
                cmd += ["--ma-values-offset", str(offset)]
            out = subprocess.check_output(
                cmd,
                cwd=os.path.dirname(binary),
                env={**os.environ, "PYTHONHASHSEED": "0"})
            runs[tag].append(json.loads(out))

    for tag in runs:
        info = runs[tag][0]["info"]
        print("%-8s %s" % (tag, info), file=sys.stderr)

    cases = [r["case"] for r in runs["baseline"][0]["results"]]
    print()
    print("| case | baseline ns/op | patched ns/op | change |")
    print("|---|---|---|---|")
    for case in cases:
        def vals(tag):
            return sorted(next(r["best_ns_per_op"] for r in run["results"]
                               if r["case"] == case)
                          for run in runs[tag])
        b, p = vals("baseline"), vals("patched")
        bm, pm = statistics.median(b), statistics.median(p)
        print("| %s | %.1f | %.1f | %+.2f%% |" % (case, bm, pm,
                                                  (pm - bm) / bm * 100.0))
    print()
    print("min/median over %d alternating runs; see stderr for build info"
          % args.runs)


def main():
    ap = argparse.ArgumentParser(description=__doc__)
    ap.add_argument("--worker", action="store_true", help=argparse.SUPPRESS)
    ap.add_argument("--ma-values-offset", type=int, default=None,
                    help=argparse.SUPPRESS)
    ap.add_argument("--baseline", help="baseline interpreter")
    ap.add_argument("--patched", help="patched interpreter")
    ap.add_argument("--runs", type=int, default=12)
    ap.add_argument("--offset-src", default=".",
                    help="CPython source/build tree with the headers")
    ap.add_argument("--no-verify", action="store_true")
    args = ap.parse_args()

    if args.worker:
        run_worker(args.ma_values_offset)
    else:
        if not args.baseline or not args.patched:
            ap.error("--baseline and --patched are required")
        run_driver(args)


if __name__ == "__main__":
    main()

[hetaozdh]

One additional idea worth discussing is preserving and reusing the intermediate result from _Py_dict_lookup() to avoid the additional Py_ssize_t hashpos = lookdict_index(mp->ma_keys, hash, ix); lookup. I'm not sure how much overhead this additional lookup introduces, though, so it would be worth benchmarking.

[eendebakpt]

CI failures seem unrelated. @hetaozdh Can you merge with main again to be sure?