Skip to content
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

Don't use eval_always for miri queries used from codegen. #65927

Merged
merged 1 commit into from Oct 29, 2019

Conversation

@eddyb
Copy link
Member

eddyb commented Oct 29, 2019

This should fix the massive incremental perf regression introduced in #65664.

It seems that eval_always was mistakenly(?) added to const_field and then it ended up on const_caller_location (which is used much more often than const_field is).

r? @michaelwoerister cc @oli-obk @nnethercote

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Oct 29, 2019

@bors rollup=never p=10 (ideally we can land it in time for the next nightly)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Oct 29, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 29, 2019

📌 Commit beb06ae has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 29, 2019

⌛️ Testing commit beb06ae with merge d3d28a4...

bors added a commit that referenced this pull request Oct 29, 2019
…lwoerister

Don't use eval_always for miri queries used from codegen.

This should fix the [massive incremental perf regression](https://perf.rust-lang.org/compare.html?start=95f437b3cfb2fec966d7eaf69d7c2e36f9c274d1&end=9285d401a6070094747465962bc49969b93e14c5&stat=instructions:u) introduced in #65664.

It seems that `eval_always` was mistakenly(?) added to `const_field` and then it ended up on `const_caller_location` (which is used much more often than `const_field` is).

r? @michaelwoerister cc @oli-obk @nnethercote
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 29, 2019

☀️ Test successful - checks-azure
Approved by: michaelwoerister
Pushing d3d28a4 to master...

@bors bors added the merged-by-bors label Oct 29, 2019
@bors bors merged commit beb06ae into rust-lang:master Oct 29, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191029.26 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@eddyb eddyb deleted the eddyb:eval-always-considered-harmful branch Oct 29, 2019
@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Oct 29, 2019

@rust-timer build eb5ef81

(I'm trying to prioritize the build just before this PR, to get the perf diff data sooner)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 29, 2019

Queued eb5ef81 with parent 2dd4e73, future comparison URL.

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Oct 29, 2019

Comparisons I'm interested in:

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Oct 29, 2019

If you look at just the check builds in the original perf regression (use the "filter" box near the top) the only entry of note is this:

packed-simd-check | avg: 2.9% | min: 2.2% | max: 4.2%

So the packed-simd regressions remain and are from #65664. The other slight regressions (e.g. piston-image-opt, keccak-debug) are likely from different PRs.

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Oct 29, 2019

Wait, check? From #65664? Are there a lot of panic!()s being expanded or something?

We went the way we did was to avoid having an (unsafe) intrinsic call from panic!, and also because the intrinsic would have to handle being used from a macro better, but I could try getting that to work tomorrow, if that regression is considered important.

EDIT: opened #65973 for that packed-simd-* regression.

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Oct 29, 2019

2-4% is non-trivial, so if there's a way to recover from it that would be great.

@rust-timer

This comment was marked as off-topic.

Copy link

rust-timer commented Oct 30, 2019

Finished benchmarking try commit eb5ef81, comparison URL.

@eddyb

This comment has been minimized.

Copy link
Member Author

eddyb commented Oct 30, 2019

Interestingly enough, packed-simd-* also regressed between the rollup and the fix.
But not as much, and not on incremental runs, so it's likely a smaller, and different, regression.
EDIT: from what data is already in perf, the smaller regression can be narrowed down to this range.
EDIT2: looking at the two rollups that make the remaining "small regression" range, #65294 is the only one that I would guess had anything to do with it, but I'm unsure.

bors added a commit that referenced this pull request Oct 30, 2019
caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!.

The main change here is to `core::panic!`, trying to fix this remaining regression: #65927 (comment)

However, in order for `caller_location` to be usable from macros the same way `file!()`/`line!()` are, it needs to have the same behavior (of extracting the macro invocation site `Span` and using that).

Arguably we would've had to do this at some point anyway, if we want to use `#[track_caller]` to replace the `file!()`/`line!()` uses from macros, but I'm not sure the RFC mentions this at all.

r? @petrochenkov cc @anp @nnethercote
Centril added a commit to Centril/rust that referenced this pull request Nov 6, 2019
…ochenkov

caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!.

The main change here is to `core::panic!`, trying to fix this remaining regression: rust-lang#65927 (comment)

However, in order for `caller_location` to be usable from macros the same way `file!()`/`line!()` are, it needs to have the same behavior (of extracting the macro invocation site `Span` and using that).

Arguably we would've had to do this at some point anyway, if we want to use `#[track_caller]` to replace the `file!()`/`line!()` uses from macros, but I'm not sure the RFC mentions this at all.

r? @petrochenkov cc @anp @nnethercote
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.