Skip to content

Conversation

@stegaBOB
Copy link
Contributor

No description provided.

@Aursen
Copy link
Contributor

Aursen commented Aug 22, 2025

Did you benchmark this kind of changes?

Copy link
Contributor

@sonicfromnewyoke sonicfromnewyoke left a comment

Choose a reason for hiding this comment

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

@Aursen i expect small performance degradation

@stegaBOB
Copy link
Contributor Author

I'll benchmark it, but I doubt this would have any performance impacts. The compiler should optimize the error branch out.

@stegaBOB
Copy link
Contributor Author

stegaBOB commented Aug 22, 2025

@Aursen @sonicfromnewyoke https://rust.godbolt.org/z/9YeWsr1oP map with try_map infallible under the hood compiles down to the same assembly, so this should have 0 performance impact. This should be trivial for the compiler to optimize. One of the reasons Infallible exists :)

@sonicfromnewyoke
Copy link
Contributor

@Aursen @sonicfromnewyoke https://rust.godbolt.org/z/9YeWsr1oP map with try_map infallible under the hood compiles down to the same assembly, so this should have 0 performance impact. This should be trivial for the compiler to optimize. One of the reasons Infallible exists :)

@stegaBOB it works even better than i expected, thanks

the last question: what do you think about inlining? Will it be better to use #[inline(always)] instead of putting the decision on the compiler??

@Aursen
Copy link
Contributor

Aursen commented Aug 22, 2025

@stegaBOB it works even better than i expected, thanks

the last question: what do you think about inlining? Will it be better to use #[inline(always)] instead of putting the decision on the compiler??

I think for this kind of choice, @febo just benchmarks

@stegaBOB
Copy link
Contributor Author

the last question: what do you think about inlining? Will it be better to use #[inline(always)] instead of putting the decision on the compiler??

I think #[inline(always)] should only really be done if you have serious benchmarks to justify it (or if the function is just empty). The compiler will probably make the right choice

@febo
Copy link
Collaborator

febo commented Aug 22, 2025

@stegaBOB it works even better than i expected, thanks
the last question: what do you think about inlining? Will it be better to use #[inline(always)] instead of putting the decision on the compiler??

I think for this kind of choice, @febo just benchmarks

In this case #[inline] seems more appropriate since we don't know what the mapping function will be – it might simple, but it might not.

febo
febo previously approved these changes Aug 26, 2025
Copy link
Collaborator

@febo febo left a comment

Choose a reason for hiding this comment

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

👌

@febo febo self-requested a review August 26, 2025 01:20
@febo febo dismissed their stale review August 26, 2025 01:21

It would be good to include a test.

@stegaBOB stegaBOB force-pushed the stegaBOB/feat/ref-try-map branch 2 times, most recently from e914238 to 906c142 Compare August 26, 2025 19:11
@stegaBOB stegaBOB force-pushed the stegaBOB/feat/ref-try-map branch from 906c142 to af6442f Compare August 26, 2025 19:14
Copy link
Collaborator

@febo febo left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@febo febo merged commit ebe6ffc into anza-xyz:main Aug 27, 2025
11 checks passed
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.

4 participants