Skip to content

Fix perf regression from Miri Machine trait changes #62264

New issue

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

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

Already on GitHub? # to your account

Merged
merged 4 commits into from
Jul 6, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 1, 2019

Maybe this fixes the perf regression that #62003 seemingly introduced?

Cc @nnethercote

@rust-highfive
Copy link
Contributor

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2019

@bors try

@bors
Copy link
Collaborator

bors commented Jul 1, 2019

⌛ Trying commit 8157856098a7038232d52a993bb945f7eafa0add with merge a36397fa11d1d3c42a1ac182d0159fa152cc005d...

@bors
Copy link
Collaborator

bors commented Jul 1, 2019

💥 Test timed out

@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2019

The build succeeded but the timeout went off... @rust-lang/infra should the timeout be increased?

Let's see if we can time this anyway.
@rust-timer build a36397fa11d1d3c42a1ac182d0159fa152cc005d

@rust-timer
Copy link
Collaborator

Success: Queued a36397fa11d1d3c42a1ac182d0159fa152cc005d with parent 5748825, comparison URL.

1 similar comment
@rust-timer
Copy link
Collaborator

Success: Queued a36397fa11d1d3c42a1ac182d0159fa152cc005d with parent 5748825, comparison URL.

@mati865
Copy link
Contributor

mati865 commented Jul 1, 2019

I think try builds will timeout until #62247 is merged.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit a36397fa11d1d3c42a1ac182d0159fa152cc005d, comparison URL.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2019

That doesn't seem to make a difference. I guess it's time to revert parts of that PR to figure out where it is coming from.

(Please don't merge!)

@RalfJung RalfJung changed the title more inlining for force_bits [WIP] Debug perf regression from Miri Machine trait changes Jul 1, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2019

@bors try

@bors
Copy link
Collaborator

bors commented Jul 1, 2019

⌛ Trying commit 269866b with merge 32136d3...

bors added a commit that referenced this pull request Jul 1, 2019
[WIP] Debug perf regression from Miri Machine trait changes

Maybe this fixes the perf regression that #62003 seemingly introduced?

Cc @nnethercote
@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2019

@rust-timer build 32136d3

@rust-timer
Copy link
Collaborator

Success: Queued 32136d3 with parent 765eebf, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 32136d3, comparison URL.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

Yeah, this confirms the perf regression is fixed by reverting the entire PR.

@nnethercote can you explain that? I am completely mystified. That just makes no sense at all. And without any hypothesis what is even causing this, it is very hard to do anything about it.^^

Next run: reverting those parts of the PR that we did just "for consistency", but don't technically need.

@bors try

@bors
Copy link
Collaborator

bors commented Jul 2, 2019

⌛ Trying commit 4c8da8b268372d1229bcdda40d2387f22af59860 with merge b33ad710d9c74907741387cf9a85c8e5b95b7e52...

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

Ah never mind, I found it.

Is there a way to abort the try build?

@kennytm
Copy link
Member

kennytm commented Jul 2, 2019

@bors try- retry

@RalfJung RalfJung changed the title [WIP] Debug perf regression from Miri Machine trait changes Fix perf regression from Miri Machine trait changes Jul 2, 2019
@bors
Copy link
Collaborator

bors commented Jul 2, 2019

⌛ Trying commit ffc6670 with merge ed4ad9c...

bors added a commit that referenced this pull request Jul 2, 2019
Fix perf regression from Miri Machine trait changes

Maybe this fixes the perf regression that #62003 seemingly introduced?

Cc @nnethercote
@bors
Copy link
Collaborator

bors commented Jul 2, 2019

☀️ Try build successful - checks-azure, checks-travis
Build commit: ed4ad9c

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

@rust-timer build ed4ad9c

@rust-timer
Copy link
Collaborator

Success: Queued ed4ad9c with parent 99abdfa, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ed4ad9c, comparison URL.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

Perf looks good, confirms my assumption that this fixes the regression. This is ready for review.

@nnethercote
Copy link
Contributor

Thank you for hunting this down, RalfJung! For compiler perf, fixing regressions is less glamorous but just as important as writing new optimizations.

@bors
Copy link
Collaborator

bors commented Jul 4, 2019

☔ The latest upstream changes (presumably #62355) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2019

Rebased to fix conflict.

@zackmdavis
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 5, 2019

📌 Commit 52e6f85 has been approved by zackmdavis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
Fix perf regression from Miri Machine trait changes

Maybe this fixes the perf regression that rust-lang#62003 seemingly introduced?

Cc @nnethercote
bors added a commit that referenced this pull request Jul 5, 2019
Rollup of 8 pull requests

Successful merges:

 - #60260 (Add support for UWP targets)
 - #62151 (Update linked OpenSSL version)
 - #62245 (Miri engine: support extra function (pointer) values)
 - #62257 (forward read_c_str method from Memory to Alloc)
 - #62264 (Fix perf regression from Miri Machine trait changes)
 - #62296 (request at least ptr-size alignment from posix_memalign)
 - #62329 (Remove support for 1-token lookahead from the lexer)
 - #62377 (Add test for ICE #62375)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 6, 2019
Fix perf regression from Miri Machine trait changes

Maybe this fixes the perf regression that rust-lang#62003 seemingly introduced?

Cc @nnethercote
bors added a commit that referenced this pull request Jul 6, 2019
Rollup of 7 pull requests

Successful merges:

 - #62151 (Update linked OpenSSL version)
 - #62245 (Miri engine: support extra function (pointer) values)
 - #62257 (forward read_c_str method from Memory to Alloc)
 - #62264 (Fix perf regression from Miri Machine trait changes)
 - #62296 (request at least ptr-size alignment from posix_memalign)
 - #62329 (Remove support for 1-token lookahead from the lexer)
 - #62377 (Add test for ICE #62375)

Failed merges:

r? @ghost
@bors bors merged commit 52e6f85 into rust-lang:master Jul 6, 2019
@RalfJung RalfJung deleted the inline-forcing branch July 6, 2019 08:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants