Skip to content

Reuse stored hashes when building a set from a set/frozenset/dict - #8491

Merged
youknowone merged 3 commits into
RustPython:mainfrom
fregataa:reuse-stored-hashes-in-set-ops
Aug 12, 2026
Merged

Reuse stored hashes when building a set from a set/frozenset/dict#8491
youknowone merged 3 commits into
RustPython:mainfrom
fregataa:reuse-stored-hashes-in-set-ops

Conversation

@fregataa

@fregataa fregataa commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Closes #8489.

Changes

dict_inner.rs — each entry point computed the hash on its first line and passed only the hash variable downward, so the split is mechanical: the body moves to a *_known_hash variant and the original becomes a wrapper. No logic is duplicated.

  • insert_known_hash, contains_known_hash, remove_if_known_hash, delete_if_exists_known_hash, delete_or_insert_known_hash
  • keys_with_hashes() — yields (key, hash) from the entries, next to the existing keys()/values()/items()

The _known_hash suffix follows CPython's "KnownHash" variants (_PyDict_SetItem_KnownHash, _PyDict_Contains_KnownHash, _PyDict_DelItem_KnownHash), which mark the same split as set_add_key/set_add_entry on the set side. keys_with_hashes() keeps with because it really does return the hashes.

function/protocol.rsArgIterable::as_object(), a pub(crate) accessor for the object before any __iter__ call. The set operations take ArgIterable, which already held the original PyObjectRef in a private field; reaching it lets them dispatch on the source type without changing a single signature (they are passed around as fn(&PySetInner, ArgIterable, &VirtualMachine) -> ... function pointers, so changing them would have rippled widely).

builtins/set.rs

  • PySetInner::cached_hashes()Some(Vec<(key, hash)>) for a set/frozenset (subclasses included, via the existing extract_set) or an exact dict, None otherwise. Same predicates as CPython.
  • add_known_hash / contains_known_hash
  • merge_set / merge_dict now forward the stored hashes
  • Fast path added to union, intersection, difference, symmetric_difference, difference_update, symmetric_difference_update, intersection_updatethe existing generic loop stays as the fallback in each
  • PyFrozenSet's Constructor::Args becomes OptionalArg<PyObjectRef> so py_new receives the source object and can hand its stored hashes over via PySetInner::from_object, instead of flattening to Vec<PyObjectRef> first; slot_new keeps only argument parsing and the empty-singleton check

Result

after dict.fromkeys(map):  10
after frozenset(d):        10
after set(d):              10
after s.difference(d):     10
after s.symmetric_difference_update(d): 10

Testing

  • test_setRan 630 tests ... OK (expected failures=10), no new failures
  • test_dict, test_dictviews, test_dictcomps, test_ordered_dict, test_defaultdict, test_userdict, test_setcomps, test_weakset, test_collections, test_copy — all pass
  • test_types, test_builtin, test_iter, test_pickle, test_marshal, test_descr, test_class, test_functools, test_typing — all pass
  • extra_tests/snippets/: builtin_set.py, builtin_dict.py, builtin_dict_union.py, frozen.py
  • Ad-hoc: empty-frozenset singleton, frozenset(f) is f, distinct ids for empty subclass instances, dict subclasses correctly excluded from the exact-dict path, self-referential ops (s.update(s), s.symmetric_difference_update(s)), results not aliasing their operands, error messages unchanged vs CPython 3.14.6
  • cargo fmt --check clean, cargo clippy -p rustpython-vm --all-targets no new warnings

Review note

insert_known_hash depends on a contract the callee cannot verify: pass a hash that isn't key.key_hash(vm) and the entry lands in a bucket no lookup will probe, so the key silently disappears. This is documented on the function, and every call site forwards a hash that came from keys_with_hashes() on a container holding that same key object. Worth a careful look.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Frozensets can now be created directly from an optional iterable.
    • Exact frozenset inputs are preserved where applicable.
    • Empty frozenset creation continues to reuse the shared empty instance.
  • Performance

    • Set operations and updates are more efficient by reusing cached element hashes.
    • Improved hashing efficiency for set, frozenset, and dictionary insertion, lookup, deletion, merging, and membership operations.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 39e7f1a4-a2ec-4725-97ec-24706c8ef7d3

📥 Commits

Reviewing files that changed from the base of the PR and between 561d16b and e27697d.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/set.rs
  • crates/vm/src/dict_inner.rs
  • crates/vm/src/function/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/vm/src/function/protocol.rs
  • crates/vm/src/dict_inner.rs
  • crates/vm/src/builtins/set.rs

📝 Walkthrough

Walkthrough

Set and frozenset operations now reuse cached hashes from sets, frozensets, and exact dictionaries. Dictionary APIs accept caller-supplied hashes. Frozenset construction accepts an optional iterable and preserves exact-object fast paths.

Changes

Cached-hash set operations

Layer / File(s) Summary
Dictionary known-hash APIs
crates/vm/src/dict_inner.rs
Dictionary insertion, lookup, deletion, conditional removal, and key enumeration now support cached hashes.
Cached-hash set operations
crates/vm/src/builtins/set.rs
Set construction, updates, and set algebra reuse hashes from sets, frozensets, and exact dictionaries.
Frozenset construction
crates/vm/src/builtins/set.rs, crates/vm/src/function/protocol.rs
PyFrozenSet accepts an optional iterable, preserves exact empty and frozenset fast paths, and builds storage from source objects.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested labels: z-ca-2026

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement issue #8489 by reusing cached hashes across set and frozenset construction and set operations.
Out of Scope Changes check ✅ Passed The dictionary helpers and iterable accessor support the scoped hash-reuse implementation and introduce no unrelated changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: reusing stored hashes when constructing sets from sets, frozensets, or dictionaries.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

set and frozenset recomputed __hash__ for every element even when the
source object already stored a hash per entry. CPython's
set_update_internal branches on PyAnySet_Check / PyDict_CheckExact and
feeds set_add_entry the hash read from the source table; RustPython
always iterated generically.

Split the hash computation out of the Dict entry points so callers can
supply a hash they already hold, add keys_with_hashes() to hand out
(key, hash) pairs, and take the fast path in the set constructors and
in the set operations whose argument is a set/frozenset/exact dict.

ArgIterable::as_object() exposes the pre-__iter__ object so the set
operations can dispatch on the source type without changing any of
their signatures.

Closes RustPython#8489. dict.fromkeys() is the dict-target counterpart and is
tracked in RustPython#8490, so test_do_not_rehash_dict_keys keeps its
expectedFailure marker until that lands too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fregataa
fregataa force-pushed the reuse-stored-hashes-in-set-ops branch from ba69646 to 2924f86 Compare August 11, 2026 12:09
`_with_hash` said nothing about which direction the hash travels, and the
same suffix was already used both ways in this file: keys_with_hashes()
hands hashes out, while insert_with_hash() takes one in. The pre-existing
insert_with_hint()/get_with_hint() pair has the same problem.

Follow CPython's "KnownHash" variants (_PyDict_SetItem_KnownHash,
_PyDict_Contains_KnownHash, _PyDict_DelItem_KnownHash) instead, so the
suffix marks the argument direction and matches the name a reader
familiar with CPython already expects. keys_with_hashes() keeps `with`
because it really does return the hashes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fregataa
fregataa marked this pull request as ready for review August 11, 2026 23:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/vm/src/builtins/set.rs`:
- Around line 467-475: Update PySetInner::update to delegate to update_internal
so set.__ior__() reuses cached hashes when merging an existing set instead of
recomputing __hash__. Preserve set.update() behavior and add a regression test
covering set |= existing_set with hash invocation tracking.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: f2bb6725-7dc2-43fe-acd5-6cc8f5ea4620

📥 Commits

Reviewing files that changed from the base of the PR and between 05a9873 and 561d16b.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/set.rs
  • crates/vm/src/dict_inner.rs
  • crates/vm/src/function/protocol.rs

Comment on lines +467 to +475
for (item, hash) in any_set.as_inner().content.keys_with_hashes() {
self.add_known_hash(item, hash, vm)?;
}
Ok(())
}

fn merge_dict(&self, dict: PyDictRef, vm: &VirtualMachine) -> PyResult<()> {
for (key, _value) in dict {
self.add(key, vm)?;
for (key, hash) in dict._as_dict_inner().keys_with_hashes() {
self.add_known_hash(key, hash, vm)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline crates/vm/src/builtins/set.rs --items all --type function --match 'update|update_internal|merge_set|merge_dict'
rg -n -C 5 --type rust 'fn update\b|\.update\(' crates/vm/src/builtins/set.rs

Repository: RustPython/RustPython

Length of output: 2076


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '400,490p' crates/vm/src/builtins/set.rs
sed -n '780,815p' crates/vm/src/builtins/set.rs
rg -n -C 4 --type rust 'enum ArgIterable|struct ArgIterable|impl ArgIterable|fn as_object|into_iterable_iter|keys_with_hashes|add_known_hash' crates/vm/src
rg -n -C 6 --type rust 'update_internal|\.inner\.update\(|inner\.update\(' crates/vm/src/builtins/set.rs crates/vm/src | head -n 240
rg -n --glob '*.rs' 'hash.*raises|raises.*hash|__hash__|Hash' Lib tests crates/vm/src/builtins/set.rs | head -n 240

Repository: RustPython/RustPython

Length of output: 25269


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path

p = Path("crates/vm/src/builtins/set.rs")
lines = p.read_text().splitlines()
for start, end in [(430, 485), (790, 810), (870, 890)]:
    print(f"--- {p}:{start}-{end} ---")
    for n in range(start, min(end, len(lines)) + 1):
        print(f"{n}:{lines[n-1]}")
PY

rg -n -C 8 --type rust 'ArgIterable' crates/vm/src | head -n 320
rg -n -C 8 --type rust 'keys_with_hashes|add_known_hash' crates/vm/src/builtins | head -n 320

Repository: RustPython/RustPython

Length of output: 32348


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '60,115p' crates/vm/src/function/protocol.rs
sed -n '1410,1450p' crates/vm/src/builtins/set.rs
rg -n -C 10 --type rust 'enum AnySet|struct AnySet|impl.*AnySet|TryFromObject.*AnySet|into_iterable_iter' crates/vm/src/builtins/set.rs crates/vm/src
rg -n --type rust 'inner\.update\(' crates/vm/src/builtins/set.rs

Repository: RustPython/RustPython

Length of output: 19080


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '70,115p' crates/vm/src/function/protocol.rs
sed -n '1380,1460p' crates/vm/src/builtins/set.rs
rg -n -C 10 --type rust 'AnySet' crates/vm/src/builtins/set.rs | head -n 260
rg -n --type rust 'inner\.update\(' crates/vm/src/builtins/set.rs

Repository: RustPython/RustPython

Length of output: 13457


Route set.__ior__() through the cached-hash path.

set.update() already calls update_internal, but set.__ior__() calls PySetInner::update, which recomputes __hash__ for elements from an existing set. Delegate PySetInner::update to update_internal and add a regression test for set |= existing_set.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/builtins/set.rs` around lines 467 - 475, Update
PySetInner::update to delegate to update_internal so set.__ior__() reuses cached
hashes when merging an existing set instead of recomputing __hash__. Preserve
set.update() behavior and add a regression test covering set |= existing_set
with hash invocation tracking.

from_object() and ArgIterable::as_object() had no caller outside their own
file and crate respectively, so drop them to private and pub(crate). The
dict_inner helpers stay pub(crate) because builtins::set calls them.

Also shorten the doc comments to match the density of the surrounding
code; only insert_known_hash keeps a real note, since a wrong hash there
corrupts the table silently.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Aug 12, 2026

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

@youknowone
youknowone merged commit 7612e68 into RustPython:main Aug 12, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set/frozenset re-hash elements when constructed from a set, frozenset, or dict

2 participants