Skip to content

A concrete type in front of the ABC check on the per-item attribute path - #3

Merged
kmoneil merged 2 commits into
mainfrom
se-031-attribute-fast-path
Aug 25, 2026
Merged

A concrete type in front of the ABC check on the per-item attribute path#3
kmoneil merged 2 commits into
mainfrom
se-031-attribute-fast-path

Conversation

@kmoneil

@kmoneil kmoneil commented Aug 24, 2026

Copy link
Copy Markdown
Owner

What changed, and why

Evaluator._attribute tested isinstance(value, Mapping), and it runs once per attribute per
item
. collections.abc.Mapping.__instancecheck__ is Python-level and dispatches into
_abc._abc_instancecheck: 99.9 ns against 27.4 ns for a C type check, measured over two
million calls. cProfile over the canonical pipeline put <frozen abc>.__instancecheck__ at
300,000 calls for 200 evaluations, about 1,500 per evaluation, with _attribute the third-largest
entry by cumulative time.

It is now isinstance(value, (dict, Mapping)). isinstance walks a tuple left to right and stops
at the first hit, so a plain dict is answered by the C type check and never enters the ABC.

No behaviour change on any mapping kind. One line in src/safeexpr/_eval.py; the rest of the
diff is tests and the receipt.

The gain

main and this branch benchmarked within the same ten minutes on one box, medians and minimums:

Row main branch median min
map 518.5 us 456.2 us +13.7% +10.4%
max_by 558.1 us 502.9 us +11.0% +11.0%
group_by 642.4 us 582.2 us +10.3% +10.6%
split_join 632.7 us 583.4 us +8.4% +9.3%
canonical_pipeline 1,844.5 us 1,717.4 us +7.4% +7.4%
lower_each 1,257.1 us 1,175.1 us +7.0% +5.3%
where_then_map 1,404.2 us 1,317.2 us +6.6% +9.4%
where 1,117.0 us 1,057.6 us +5.6% +6.0%
nested_predicate 14,587.3 us 13,820.8 us +5.5% +5.0%
pluck 128.7 us 131.3 us -1.9% -0.8%
bare_comparison 16.7 us 16.7 us +0.0% +1.3%

The shape is the argument rather than any single row. pluck and bare_comparison are the
controls and both sit still
: pluck reads a key directly and walks no attribute per item, and
bare_comparison does one attribute in the whole expression. Every row that reads a field per item
moves, by roughly the fraction of its per-item work that was the ABC call.

Separately, three arms interleaved within one process over fifteen rounds, four independent runs,
against an ABC-only arm built by rewriting the shipped handler's own source: 6 to 11%, with the
method and the full table in tests/benchmarks/test_attribute_path_bench.py.

The guard is a call count, not a benchmark

Reordering the tuple to (Mapping, dict) gives the entire gain back and changes no behaviour, so
nothing would fail. A timing gate would not catch it either: this box's own noise floor is above
10%, which tests/benchmarks/test_scalar_tiers_bench.py already records.

So the two tests that actually protect this are counts and source reads, in tests/test_eval.py:

  • test_a_plain_dict_never_reaches_the_abc swaps _eval's module-level Mapping for a counting
    probe and asserts zero consultations over a 200-row map(_.name).
  • test_a_mapping_that_is_not_a_dict_still_reaches_the_abc asserts 200, so the test above
    cannot pass on a guard that dropped the ABC half entirely.
  • test_the_concrete_type_is_tested_before_the_abc reads the tuple's order off the source.

Verified by making the changes they exist to catch. Reordering the tuple fails two of them;
removing the fast path fails the same two.

Regression battery

Every mapping kind a host might realistically pass, in tests/test_eval.py:

  • test_regression_attribute_a_non_dict_mapping_still_reads_its_keys over ChainMap,
    MappingProxyType and a custom collections.abc.Mapping. This is the test that catches the
    guard being simplified to its concrete half, which is silent and data-dependent.
  • test_regression_attribute_a_dict_subclass_reads_its_keys over OrderedDict, Counter,
    defaultdict.
  • test_regression_attribute_a_missing_field_reports_the_same_message_on_both_paths, including
    the did-you-mean text.
  • test_regression_attribute_a_mapping_key_still_wins_over_a_method -- d.items is the key
    "items", never dict.items, on both halves.
  • test_regression_attribute_a_dunder_is_still_blocked_on_both_paths.
  • test_regression_attribute_a_non_mapping_still_needs_registration.
  • test_regression_attribute_a_mapping_subclass_cannot_reach_getattr -- a Mapping carrying a
    real attribute named like a missing key returns "no field", not the attribute.

Plus a Hypothesis differential in tests/test_transform_properties.py over the existing TREES
strategy, asserting every mapping kind agrees with a plain dict on value, error type and message.

Two things worth knowing beyond this PR

Overriding a node handler on Evaluator, or on a subclass, silently has no effect. _DISPATCH
is built in the class body and binds the original function objects, so _eval keeps calling the
unpatched handler. An A/B written that way measures 0.5% and is wrong by a factor of twenty.
Anything swapping a handler must patch Evaluator._DISPATCH.

Counter is not a drop-in for dict in a differential. Counter.__missing__ returns 0, so
c["absent"] is a value and not a KeyError. That is the standard library deciding about its own
mapping, through the same value[key] an ordinary program uses. It is pinned by
test_a_mapping_that_answers_a_missing_key_is_allowed_to rather than excluded silently.

A finding for the benchmark lane

The 10% gate in CLAUDE.md compares mean, and mean is unusable on this workload. Against a
baseline taken minutes earlier on the same machine, mean reports pluck regressing 45% and
bare_comparison 14% for a change that provably cannot touch either. bare_comparison
recorded a max of 2,278 us against a min of 15.5 us in one run, a 147x outlier, and one of those in
twenty thousand rounds moves the mean further than the effect being measured. Median and minimum
agree within ~3 points on every row above.

Whatever threshold the benchmark lane picks, it should gate on median or min.

Checks

  • ruff check, ruff format --check, mypy --strict clean
  • Full suite: 2,347 passed, 6 skipped, 58.46 s (2,281 before, so 66 new tests, no
    measurable change to runtime)
  • Coverage: 99.64% line, unchanged. Both branches of the new guard exercised.
  • Benchmarks run and compared against main on the same machine, table above
  • No registry entry touched, so the registry review gate does not apply

Merge order: this one first, then the compile-cache work, then the measurement lane.

`_attribute` ran `isinstance(value, Mapping)` once per attribute per item.
`Mapping.__instancecheck__` is Python-level and dispatches into
`_abc._abc_instancecheck`: 99.9ns against 27.4ns for a C type check, and
cProfile put it at ~1,500 calls per evaluation of the canonical pipeline.

`isinstance(value, (dict, Mapping))` answers a plain dict on the first entry
and never enters the ABC. Measured 6 to 11% on the collections tier by
interleaving the arms within one process over fifteen rounds, four runs.

The change was specified as `type(value) is dict`, on a reading in which
`isinstance(v, dict)` here was worth nothing. That did not reproduce: four
interleaved runs put the three spellings inside each other's spread on plain
dicts, and 7 to 11% apart on rows of `OrderedDict`, which only `isinstance`
catches. The tuple form is also already the house pattern, beside
`_guards._SIZED` and `_guards.HASHABLE_CONTAINERS`.

Reordering the tuple gives all of it back and changes no behaviour, so the
guard is a call count rather than a timing gate: a plain dict must reach the
ABC zero times and a ChainMap once per row, and the order is read off the
source. Both fail on a reorder, verified by making one.

Claude-Session: https://claude.ai/code/session_01Esnm9mNDpqRWf4QAVHCwHo
Medians against the previous commit on one box: map +13.7%, max_by +11.0%,
group_by +10.3%, canonical_pipeline +7.4%. pluck and bare_comparison are the
controls and both sit still, which is what makes the rest an attribution.

Claude-Session: https://claude.ai/code/session_01Esnm9mNDpqRWf4QAVHCwHo
@kmoneil
kmoneil merged commit 57297c8 into main Aug 25, 2026
12 checks passed
@kmoneil
kmoneil deleted the se-031-attribute-fast-path branch August 25, 2026 11:28
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.

1 participant