[WIP] Make usize overflow always have debug-assertions semantics #58475
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @oli-obk (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(Ok, I tried to use Github's draft features, but it appears that it doesn't run CI, oh well.) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Test failures are examples of LTO + linking with C that die with This is obviously a bug and needs to be fixed, but I don't think it's a blocker to doing any performance measurement on the impact of this. |
|
Triggering perf needs a try build, which AFAIK does need to pass tests. Maybe just delete the tests that are failing for now, since we just need something that runs, not something ready to merge? |
|
@bors try |
|
⌛ Trying commit 5680ee2bdc557143d41e3746d19831b3dafd1110 with merge 7231393efd7d34ee85e8f21b5f969232c3df0c8e... |
|
☀️ Test successful - checks-travis |
|
@rust-timer build 7231393efd7d34ee85e8f21b5f969232c3df0c8e |
|
Success: Queued 7231393efd7d34ee85e8f21b5f969232c3df0c8e with parent f47ec2a, comparison URL. |
|
Finished benchmarking try commit 7231393efd7d34ee85e8f21b5f969232c3df0c8e |
|
So the regressions here are frankly much larger than I'd expected. Since the only benchmark this really tests is the compiler, before I dig too far into looking at the code generated, are there any reasons the changes I made here would be particularly slow, even if they'd left the generated code the same? |
|
If most of the overflow checks on simple usize increments done in hot loops are not optimized away, then I would expect that to be the biggest perf killer. Are there some small functions with tight loops we could look at the before and after assembly for? |
|
Ok, one thing to note, comparing instructions (which is the default) is probably a mistake, we're for sure adding some instructions. The question is whether those instructions map to time. So for now I'm going to be looking at cycles, which seems like probably the best thing to consider. It's not a lot better from this perspective, but it is a bit: https://perf.rust-lang.org/compare.html?start=f47ec2ad5b6887b3d400aee49e2294bd27733d18&end=7231393efd7d34ee85e8f21b5f969232c3df0c8e&stat=cycles%3Au |
This is a prototype for rust-lang/rfcs#2635 to enable us to collect some measurements.
The failures aren't central to what this patch is about (they're related to linking and panic not being included in the final artifact), so they shouldn't disrupt perf measurements.
alex
pushed a commit
to alex/rust
that referenced
this pull request
Feb 22, 2019
Instead of (&[u8], position) simply store the &[u8] and reslice. This was originally written for rust-lang#58475, to see if removing the arithmetic helped with avoiding integer overflow checks, however I think the result is slightly more readable in general -- specifically the removal of set_position is a nice win. I think this might be a hair faster even without the changes in rust-lang#58475, but I haven't measured that.
Instead of (&[u8], position) simply store the &[u8] and reslice. This was originally written for rust-lang#58475, to see if removing the arithmetic helped with avoiding integer overflow checks, however I think the result is slightly more readable in general -- specifically the removal of set_position is a nice win. I think this might be a hair faster even without the changes in rust-lang#58475, but I haven't measured that.
alex
force-pushed
the
usize-overflow-panic
branch
from
5680ee2 to
864dd3c
Compare
|
@oli-obk Would you mind kicking off another try + perf run? |
|
@bors try |
bors
added a commit
that referenced
this pull request
Feb 23, 2019
[WIP] Make usize overflow always have debug-assertions semantics This is a prototype for rust-lang/rfcs#2635 to enable us to collect some measurements.
|
☀️ Test successful - checks-travis |
|
@rust-timer build abc6e24 |
|
Success: Queued abc6e24 with parent 02a4e27, comparison URL. |
|
Finished benchmarking try commit abc6e24 |
|
ping from triage @alex you have a few conflicts to resolve |
Dylan-DPC-zz
added
S-waiting-on-author
|
I'm currently working through profiling and finding places to optimize (e.g. #58692) to make this practical, so I have not been worried about mergability for the time being. |
|
Ah okay. That's fine :) |
TimNN
added
A-allocators
Centril
added
S-inactive
|
Closing due to inactivity, feel free to reopen whenever. |
|
In case anyone is eagerly following after this, I wanted to report that I had not forgotten about this! The first overflow optimization issue in LLVM is resolved, now I'm on to failing tests because of inability to do loop idiom recognition: https://bugs.llvm.org/show_bug.cgi?id=46057 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a prototype for rust-lang/rfcs#2635 to enable us to collect some measurements.