Skip to content

Preallocate Rust sets when extracting Python sets#6225

Open
charliermarsh wants to merge 5 commits into
PyO3:mainfrom
charliermarsh:charlie/preallocate-python-set-extraction
Open

Preallocate Rust sets when extracting Python sets#6225
charliermarsh wants to merge 5 commits into
PyO3:mainfrom
charliermarsh:charlie/preallocate-python-set-extraction

Conversation

@charliermarsh

@charliermarsh charliermarsh commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

When extracting a Python set or frozenset into a Rust HashSet, we currently collect a fallible iterator. Collecting into Result loses the lower bound from the source iterator's size hint, so the set grows repeatedly even though its exact length is known. In other words, because it's fallible, Rust can't pre-size the collection.

This PR instead preallocates from the Python set's length before extracting and inserting each element. (This matches the existing pattern used for for Vec, SmallVec, and Python dictionaries into HashMap, all of which already reserve capacity from the Python collection's length.)

For a 100,000-element set, this reduces allocations from 16 to 1 and cumulative Rust allocation traffic from 2,359,388 bytes to 1,179,656 bytes.

@charliermarsh
charliermarsh marked this pull request as ready for review July 22, 2026 17:48
@codspeed-hq

codspeed-hq Bot commented Jul 22, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 17.83%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 138 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
extract_hashset 23.6 ms 19.1 ms +23.52%
extract_hashbrown_set 19.1 ms 17 ms +12.41%

Tip

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


Comparing charliermarsh:charlie/preallocate-python-set-extraction (474fe00) with main (2e49529)

Open in CodSpeed

@mejrs

mejrs commented Jul 22, 2026

Copy link
Copy Markdown
Member

cast::<PySet> can also return a subclass of PySet, so len could potentially be overridden and buggy and oom us here.

Copy link
Copy Markdown
Contributor Author

I could be wrong, but AFAICT, this ultimately calls PySet_Size which returns the actual size (and __len__ is never invoked). Separately, wouldn't that concern also apply to lists, which already use this behavior?

@chirizxc

Copy link
Copy Markdown
Contributor

Is this a GitHub bug? I got the same message three times

@Tpt Tpt 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.

Thanks!

@charliermarsh

Copy link
Copy Markdown
Contributor Author

Is this a GitHub bug? I got the same message three times.

Ugh, sorry, no -- I'm on airplane Wi-Fi and I think it re-sent a bunch of times!

@bschoenmaeckers

Copy link
Copy Markdown
Member

cast::<PySet> can also return a subclass of PySet, so len could potentially be overridden and buggy and oom us here.

I could be wrong, but AFAICT, this ultimately calls PySet_Size which returns the actual size (and __len__ is never invoked). Separately, wouldn't that concern also apply to lists, which already use this behavior?

FYI our PyList iterator is also buggy (see #6087), so I'm inclined to allow this. Or find a solution for both list and sets.

Comment thread src/conversions/std/set.rs
Comment thread src/conversions/std/set.rs
@Tpt

Tpt commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@charliermarsh Glad to merge if you think the comments have been addressed

@charliermarsh

Copy link
Copy Markdown
Contributor Author

Just marked comments as resolved, thanks @Tpt!

@Tpt
Tpt added this pull request to the merge queue Jul 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 23, 2026
@Tpt
Tpt added this pull request to the merge queue Jul 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 23, 2026
@Tpt

Tpt commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

The CI failure is unrelated: ruff (😉) now tries to reformat code snippets in markdown and we have not pinned its version. Wil lfix tomorrow

@chirizxc

chirizxc commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

The CI failure is unrelated: ruff (😉) now tries to reformat code snippets in markdown and we have not pinned its version. Wil lfix tomorrow

It seems worth pinning all versions of the tools in the dependency groups, including zizmor, typos, etc.

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.

6 participants