Reuse stored hashes when building a set from a set/frozenset/dict - #8491
Draft
fregataa wants to merge 1 commit into
Draft
Reuse stored hashes when building a set from a set/frozenset/dict#8491fregataa wants to merge 1 commit into
fregataa wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
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
force-pushed
the
reuse-stored-hashes-in-set-ops
branch
from
August 11, 2026 12:09
ba69646 to
2924f86
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8489.
Changes
dict_inner.rs— each entry point computed the hash on its first line and passed only thehashvariable downward, so the split is mechanical: the body moves to a*_with_hashvariant and the original becomes a wrapper. No logic is duplicated.insert_with_hash,contains_with_hash,remove_if_with_hash,delete_if_exists_with_hash,delete_or_insert_with_hashkeys_with_hashes()— yields(key, hash)from the entries, next to the existingkeys()/values()/items()These mirror CPython's
_key/_entrypairs (set_add_key/set_add_entry,set_contains_key/set_contains_entry).function/protocol.rs—ArgIterable::as_object()returns the object before any__iter__call. The set operations takeArgIterable, which already held the originalPyObjectRefin a private field; exposing it lets them dispatch on the source type without changing a single public signature (they are passed around asfn(&PySetInner, ArgIterable, &VirtualMachine) -> ...function pointers, so changing them would have rippled widely).builtins/set.rsPySetInner::cached_hashes()—Some(Vec<(key, hash)>)for a set/frozenset (subclasses included, via the existingextract_set) or an exact dict,Noneotherwise. Same predicates as CPython.add_with_hash/contains_with_hashmerge_set/merge_dictnow forward the stored hashesunion,intersection,difference,symmetric_difference,difference_update,symmetric_difference_update,intersection_update— the existing generic loop stays as the fallback in eachPyFrozenSet'sConstructor::ArgsbecomesOptionalArg<PyObjectRef>sopy_newreceives the source object and can hand its stored hashes over viaPySetInner::from_object, instead of flattening toVec<PyObjectRef>first;slot_newkeeps only argument parsing and the empty-singleton checkResult
Testing
test_set—Ran 630 tests ... OK (expected failures=10), no new failurestest_dict,test_dictviews,test_dictcomps,test_ordered_dict,test_defaultdict,test_userdict,test_setcomps,test_weakset,test_collections,test_copy— all passtest_types,test_builtin,test_iter,test_pickle,test_marshal,test_descr,test_class,test_functools,test_typing— all passextra_tests/snippets/:builtin_set.py,builtin_dict.py,builtin_dict_union.py,frozen.pyfrozenset(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.6cargo fmt --checkclean,cargo clippy -p rustpython-vm --all-targetsno new warningsReview note
insert_with_hashdepends on a contract the callee cannot verify: pass a hash that isn'tkey.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 fromkeys_with_hashes()on a container holding that same key object. Worth a careful look.🤖 Generated with Claude Code