Skip to content

Inline next of ToQuads iterator #460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2025
Merged

Inline next of ToQuads iterator #460

merged 1 commit into from
Jul 4, 2025

Conversation

@LaurenzV
Copy link
Contributor

@LaurenzV LaurenzV commented Jul 4, 2025

Turns out that it doesn't seem to be inlined by default, resulting in worse performance. Running 50 iterations of the same code results in:

Before:

Runtime 43.567833ms
Runtime 38.34925ms
Runtime 37.596917ms
Runtime 38.616417ms
Runtime 37.788833ms
Runtime 37.371542ms
Runtime 37.517917ms
Runtime 37.289084ms
Runtime 37.520334ms
Runtime 37.314875ms
Runtime 37.617ms
Runtime 37.747334ms
Runtime 37.413583ms
Runtime 37.400958ms
Runtime 36.614334ms
Runtime 36.664042ms
Runtime 36.761125ms
Runtime 36.561958ms
Runtime 37.3525ms
Runtime 37.019084ms
Runtime 37.444208ms
Runtime 37.21775ms
Runtime 37.591209ms
Runtime 38.096292ms
Runtime 38.308125ms
Runtime 37.68775ms
Runtime 38.043458ms
Runtime 37.389208ms
Runtime 38.305417ms
Runtime 38.054958ms
Runtime 38.363709ms
Runtime 39.003625ms
Runtime 38.487375ms
Runtime 38.191ms
Runtime 38.0725ms
Runtime 38.612625ms
Runtime 38.468041ms
Runtime 38.426375ms
Runtime 38.604541ms
Runtime 37.95275ms
Runtime 38.590166ms
Runtime 38.905541ms
Runtime 38.377291ms
Runtime 38.230875ms
Runtime 38.727834ms
Runtime 38.440917ms
Runtime 37.6715ms
Runtime 37.801708ms
Runtime 38.937792ms
Runtime 39.03125ms

After:

Runtime 41.434167ms
Runtime 35.620667ms
Runtime 36.187958ms
Runtime 35.689125ms
Runtime 34.843583ms
Runtime 35.98075ms
Runtime 35.437167ms
Runtime 35.726084ms
Runtime 36.016334ms
Runtime 36.802125ms
Runtime 36.224875ms
Runtime 35.603666ms
Runtime 36.0035ms
Runtime 35.889458ms
Runtime 35.708042ms
Runtime 34.672375ms
Runtime 35.625416ms
Runtime 36.022791ms
Runtime 36.068416ms
Runtime 34.711708ms
Runtime 35.657917ms
Runtime 35.406041ms
Runtime 34.743209ms
Runtime 34.100666ms
Runtime 34.8835ms
Runtime 34.95975ms
Runtime 34.7145ms
Runtime 35.5855ms
Runtime 34.943875ms
Runtime 35.831167ms
Runtime 34.904875ms
Runtime 34.779125ms
Runtime 35.945791ms
Runtime 35.915917ms
Runtime 35.527167ms
Runtime 35.689542ms
Runtime 36.088625ms
Runtime 35.810416ms
Runtime 35.808125ms
Runtime 36.035333ms
Runtime 35.379292ms
Runtime 36.020958ms
Runtime 35.328709ms
Runtime 35.740625ms
Runtime 35.262084ms
Runtime 35.84775ms
Runtime 36.114708ms
Runtime 35.384834ms
Runtime 34.386333ms
Runtime 35.307417ms

tomcur
tomcur approved these changes Jul 4, 2025
View reviewed changes
Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

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

Makes sense to inline this. Rust doesn't inline across crate boundaries by default.

@LaurenzV LaurenzV added this pull request to the merge queue Jul 4, 2025
@LaurenzV
Copy link
Contributor Author

LaurenzV commented Jul 4, 2025

The struct is only used internally, so that's not the problem I think. But I think it's currently used in two places (and also in some tests), that's probably why.

Merged via the queue into main with commit d674833 Jul 4, 2025
15 checks passed
@LaurenzV LaurenzV deleted the inline branch July 4, 2025 09:28
@tomcur
Copy link
Member

tomcur commented Jul 4, 2025

The struct is only used internally, so that's not the problem I think. But I think it's currently used in two places (and also in some tests), that's probably why.

Ah, interesting. Which function were you calling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

3 participants

@LaurenzV @tomcur