Skip to content

Reuse stored hashes for frozenset hashing and in-place set union - #8859

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:set_remaining_hashes
Sep 27, 2026
Merged

Reuse stored hashes for frozenset hashing and in-place set union#8859
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:set_remaining_hashes

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Sep 27, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Compute the first frozenset hash from stored element hashes without calling element __hash__ methods again. In-place set union uses the same native merge path as set.update, including for set and frozenset subclasses.

Follow-up to #8491, which introduced stored-hash reuse in set construction.

Performance

Release build, 512 keys; median of three samples.

Operation Keys Before After Speedup
In-place union 4-item tuples 39.51 µs 16.24 µs 2.43×
In-place union 16-item tuples 75.40 µs 16.30 µs 4.63×
Build and hash frozenset 4-item tuples 32.68 µs 16.65 µs 1.96×
Build and hash frozenset 16-item tuples 66.73 µs 16.80 µs 3.97×

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Set and frozen-set operations now reuse hashes already stored for their elements, avoiding unnecessary rehashing.
    • In-place union now handles sets and frozen sets correctly, including subclasses with customized iteration, and avoids adding duplicate elements when a set is unioned with itself.
    • Frozen-set hashes remain consistent regardless of the order in which elements were added.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: c1cac50d-1524-4135-b8ef-84c54b00f49d

📥 Commits

Reviewing files that changed from the base of the PR and between f871707 and ce427b2.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/set.rs
  • crates/vm/src/dict_inner.rs
  • extra_tests/snippets/builtin_set.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

Set hashing now folds stored element hashes, and in-place union merges set sources directly. Regression tests cover hash reuse, union with set and frozenset sources, and self-union.

Changes

Stored-hash set operations

Layer / File(s) Summary
Stored-hash hashing and union
crates/vm/src/dict_inner.rs, crates/vm/src/builtins/set.rs, extra_tests/snippets/builtin_set.py		 Dict::fold_hashes visits stored hashes from live entries. Frozenset hashing uses those hashes, and in-place union merges set sources directly. Regression tests check that these operations do not rehash elements or invoke overridden iteration.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ce427

No actionable issue is established; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ce427

The changed collection operations stay within the existing set and dictionary storage model. The reviewed paths show no new trust boundary or broader authority, but unusual re-entrant and concurrent behavior has not been fully validated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Caller-supplied elements and set subclasses can reach these Python collection operations, but the inspected change stays within VM collection storage rather than adding service, tenant, credential, or infrastructure authority.

Trust Boundaries and Controls

  • observed — The native source check admits set and frozenset subclasses; other inputs to the shared update path continue through VM-mediated iteration and insertion. Stored hashes come from collection entries rather than a caller-supplied hash argument to in-place union.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes both main changes: reusing stored hashes for frozenset hashing and using native paths for in-place set union.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@codspeed

codspeed Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will improve performance by 27.72%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ rustpython[frozenset.py] 456.4 µs 357.3 µs +27.72%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

Comparing 1ndahous3:set_remaining_hashes (ce427b2) with main (f871707)

Open in CodSpeed

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@youknowone
youknowone enabled auto-merge (squash) September 27, 2026 15:06
@youknowone
youknowone merged commit 9fd6d76 into RustPython:main Sep 27, 2026
22 checks passed
@1ndahous3
1ndahous3 deleted the set_remaining_hashes branch September 27, 2026 17:40
@1ndahous3 1ndahous3 mentioned this pull request Sep 27, 2026
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1ndahous3 @youknowone