-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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.
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
Use log10 for optimizations #89737
Use log10 for optimizations #89737
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Insufficient permissions to issue commands to rust-timer. |
@r00ster91: 🔑 Insufficient privileges: not in try users |
May I have a benchmark run on this? Specifically for this |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Same error. As I suspected, the changes done in |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue The floating point |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 9df17ffb5c33eb34ffca5b225027c167cdf80ccf with merge ad3bd17c19765c771c75e437c77cc66e4a476095... |
☀️ Try build successful - checks-actions |
Queued ad3bd17c19765c771c75e437c77cc66e4a476095 with parent 0c87288, future comparison URL. |
Finished benchmarking commit (ad3bd17c19765c771c75e437c77cc66e4a476095): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
Feel free to ping me or another team member in the future - I wouldn't have seen this unless I was idly glancing through PRs. |
@r00ster91 I suspect the new |
Ah, ok, sure!
Oh, okay. Maybe that's it. I will try |
This comment has been minimized.
This comment has been minimized.
Doesn't look like that fixed it. The additional changes I made in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #91275) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -2,4 +2,4 @@ | |||
|
|||
include!("primitive/primitive-generic-impl.rs"); | |||
|
|||
// @has foo/primitive.i32.html '//div[@id="impl-ToString"]//h3[@class="code-header in-band"]' 'impl<T> ToString for T' | |||
// @has foo/primitive.i32.html '//div[@id="ToString-1"]//h3[@class="code-header in-band"]' 'impl<T> ToString for T' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change? This seems like a bug in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #89737 (comment) and #89737 (comment)
I will resolve the conflicts now and see if it's fixed. I will just remove changes to this file altogether because yeah I shouldn't have to change anything there.
This comment has been minimized.
This comment has been minimized.
4c61864
to
edae5cd
Compare
This comment has been minimized.
This comment has been minimized.
Removed the specializations and stuff, those things were probably not worth it anyway. |
☔ The latest upstream changes (presumably #92353) made this pull request unmergeable. Please resolve the merge conflicts. |
@r00ster91 if you can resolve the conflicts we can get this reviewed quickly after that |
@rustbot ready |
r? rust-lang/libs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let T-libs review this.
☔ The latest upstream changes (presumably #94706) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit lost in the long comment history above. It seems like people thought this should be faster, but it turned out to be slower when measured, and after a few rounds of code changes there has been no new benchmarking presented after the most recent code change (in which "specializations and stuff" was removed).
What benchmark is the most recent revision of the PR based on?
If it isn't something already checked into the repo, would it be possible to get that benchmark added to either library/core/benches/num or library/core/benches/fmt?
It is probably best to just close this because the remaining changes are just macro formatting (which I expect to be automated at some point anyway) and very minor changes that turn |
I would be happy to accept your macro formatting changes in a new PR. |
I think the new integer
log10
introduced in #70887 has some great potential to be used for some optimization (and the already existing{f32, f64}::log10
too) regarding integer length counting.If this is benchmarked and doesn't really result in improvements or even results in regressions, I'd leave this open and wait for #88788 to be merged and then try again, but judging by my own testing, there seem to be some improvements already: https://rust.godbolt.org/z/EexoddGhe
impl ToString for u8
andimpl ToString for i8
with an extra benchmark run after the other stuff is donecount_digits
that contains this functionality, and I think it does make sense. Maybe I should open an issue for that? (See https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20a.20new.20function.20to.20std)since = "..."
FIXME
).FIXME
s to later change somelog10
s to things like(-i16::MAX).to_string().len()
.For now this is just a draft to see if this does anything.