Don't actually create a full MIR stack frame when not needed #57351

oli-obk · 2019-01-05T12:05:47Z

This should significantly reduce overhead during const propagation and reduce overhead after copy propagation (cc #36673)

oli-obk · 2019-01-05T12:05:58Z

@bors try

bors · 2019-01-05T12:06:10Z

⌛ Trying commit c0f0f45 with merge 0ca2dc0...

@dotdash

Don't actually create a full MIR stack frame when not needed r? @dotdash This should significantly reduce overhead during const propagation and reduce overhead *after* copy propagation (cc #36673)

bors · 2019-01-05T14:36:00Z

☀️ Test successful - status-travis
State: approved= try=True

dotdash · 2019-01-05T21:23:06Z

Together with my (still WIP) fixes to the copy propagation pass, this makes enabling copy propagation pretty much free for the test case given in #36673

I don't feel comfortable to r+ as I have essentially no idea how const eval actually works.

oli-obk · 2019-01-06T17:14:36Z

This is actually not a part of const eval, but of reading fields from evaluated constants which is used for pattern matching and const propagation

cc @RalfJung do you want to take this?

oli-obk · 2019-01-07T08:35:47Z

@rust-timer build 0ca2dc0

rust-timer · 2019-01-07T08:35:49Z

Success: Queued 0ca2dc0 with parent 2fba17f, comparison URL.

rust-timer · 2019-01-07T10:34:22Z

Finished benchmarking try commit 0ca2dc0

oli-obk · 2019-01-07T11:42:20Z

Looks like a 1-2% perf improvement on a bunch of crates with only tuple-stress-check showing a regression (2% in clean incremental-check)

RalfJung · 2019-01-07T17:17:53Z

src/librustc_mir/const_eval.rs

+        Vec::new(), // upvar_decls
+        DUMMY_SP, // span
+        Vec::new(), // control_flow_destroyed
+    );


So this function is only ever called when we don't even want to execute any code? But then why even bother pushing a stack frame at all...? Basically I don't understand why this even works.

That entire mir variable is "fake" and takes no input, right? This could be made clearer by having a separate function to construct it.

How does this relate to mk_borrowck_eval_cx, which creates a "less fake" frame but also for some reason cannot use push_stack_frame. Could this function also just directly push to the stack instead?

So this function is only ever called when we don't even want to execute any code? But then why even bother pushing a stack frame at all...? Basically I don't understand why this even works.

We do call all kinds of methods on the ecx that check the current stack frame for e.g. substs and other info. These methods would fail without a stack frame. I remember talking about this before (not sure with whom), and we decided not to hack some sort of global substitutions into the ecx.

How does this relate to mk_borrowck_eval_cx, which creates a "less fake" frame but also for some reason cannot use push_stack_frame. Could this function also just directly push to the stack instead?

Jup, I unified it, good idea. This works much better.

We do call all kinds of methods on the ecx that check the current stack frame for e.g. substs and other info. These methods would fail without a stack frame.

I know.

But somewhere we also run some actual code, and then we need a real stack frame. I guess neither of these two mk*_eval_cx functions is called then?

Could you please add doc comments to all of these mk*_eval_cx functions explaining when they should be called, what the difference between them is, and what can and cannot be done with the context they return (e.g., you can project to fields of a constant etc., but you cannot actually run any code)?

RalfJung · 2019-01-07T17:18:23Z

r? @RalfJung

with only tuple-stress-check showing a regression (2% in clean incremental-check)

How is that even possible? Isn't this doing strictly less work?
Clear Incremental is a very short benchmark though so this might very well be spurious.

Mark-Simulacrum · 2019-01-07T17:20:09Z

I suspect the 2% regression on tuple-stress is indeed spurious (we could try to confirm be re-running try and perf; but I don't personally think there's really any need to -- 2% is small anyway).

src/librustc_mir/const_eval.rs

RalfJung · 2019-01-09T15:40:16Z

@bors r+

bors · 2019-01-09T15:40:17Z

📌 Commit dec79e4 has been approved by RalfJung

bors · 2019-01-10T06:03:02Z

⌛ Testing commit dec79e4 with merge d5c50f5d9e85cfc8c603e6b80500f6a044d99e40...

bors · 2019-01-10T06:03:11Z

💔 Test failed - status-appveyor

RalfJung · 2019-01-10T07:54:13Z

Log is empty. Probably spurious.

@bors retry

bors · 2019-01-10T07:54:28Z

⌛ Testing commit dec79e4 with merge 431eb66b636a03a24ef2159211e378dd064f515a...

bors · 2019-01-10T09:42:12Z

💔 Test failed - checks-travis

RalfJung · 2019-01-10T09:49:29Z

Eh, aha?

[01:43:39] test vec::test_swap_remove_empty ... ok
[01:43:40] died due to signal 11
[01:43:40] �[0m�[0m�[1m�[31merror:�[0m test failed, to rerun pass '--test collectionstests'
[01:43:40] 
[01:43:40] 
[01:43:40] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--"
[01:43:40] expected success, got: exit code: 3

No idea. (And the "raw log" view makes it impossible to even tell the number of failed tests, or so.)

pietroalbini · 2019-01-10T09:57:47Z

@bors retry #55861

@dotdash

Don't actually create a full MIR stack frame when not needed r? @dotdash This should significantly reduce overhead during const propagation and reduce overhead *after* copy propagation (cc rust-lang#36673)

@dotdash

Don't actually create a full MIR stack frame when not needed r? @dotdash This should significantly reduce overhead during const propagation and reduce overhead *after* copy propagation (cc rust-lang#36673)

@ghost

Rollup of 16 pull requests Successful merges: - #57351 (Don't actually create a full MIR stack frame when not needed) - #57353 (Optimise floating point `is_finite` (2x) and `is_infinite` (1.6x).) - #57412 (Improve the wording) - #57436 (save-analysis: use a fallback when access levels couldn't be computed) - #57453 (lldb_batchmode.py: try `import _thread` for Python 3) - #57454 (Some cleanups for core::fmt) - #57461 (Change `String` to `&'static str` in `ParseResult::Failure`.) - #57473 (std: Render large exit codes as hex on Windows) - #57474 (save-analysis: Get path def from parent in case there's no def for the path itself.) - #57494 (Speed up item_bodies for large match statements involving regions) - #57496 (re-do docs for core::cmp) - #57508 (rustdoc: Allow inlining of reexported crates and crate items) - #57547 (Use `ptr::eq` where applicable) - #57557 (resolve: Mark extern crate items as used in more cases) - #57560 (hygiene: Do not treat `Self` ctor as a local variable) - #57564 (Update the const fn tracking issue to the new metabug) Failed merges: r? @ghost

Don't actually create a full MIR stack frame when not needed

c0f0f45

rust-highfive assigned dotdash Jan 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2019

dotdash mentioned this pull request Jan 6, 2019

Performance of helloworld5000 could be improved #50994

Closed

RalfJung reviewed Jan 7, 2019

View reviewed changes

rust-highfive assigned RalfJung and unassigned dotdash Jan 7, 2019

oli-obk added 2 commits January 8, 2019 10:54

Manually push a stack frame where no valid frame is needed

14e662d

Make mk_eval_cx private to const eval

cea282b

RalfJung reviewed Jan 9, 2019

View reviewed changes

src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved

oli-obk added 3 commits January 9, 2019 16:08

Document the mk_*_eval_cx functions

f174b73

Explain the arguments of the mk_*_eval_cx functions

cd5a9e0

Not seeing the forest because there are too many trees in the way

dec79e4

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 Jan 9, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2019

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 Jan 10, 2019

RalfJung mentioned this pull request Jan 10, 2019

Simplify ConstValue::ScalarPair #57442

Merged

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2019

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 Jan 10, 2019

pietroalbini mentioned this pull request Jan 12, 2019

Rollup of 15 pull requests #57554

Closed

Centril mentioned this pull request Jan 13, 2019

Rollup of 15 pull requests #57561

Closed

Centril mentioned this pull request Jan 13, 2019

Rollup of 16 pull requests #57568

Merged

bors merged commit dec79e4 into rust-lang:master Jan 13, 2019

pnkfelix mentioned this pull request Jan 28, 2019

Project fails (or takes long time) to compile on latest stable and nightly (hang/stackoverflow) #57735

Closed

oli-obk deleted the cheap_const_ops branch June 15, 2020 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't actually create a full MIR stack frame when not needed #57351

Don't actually create a full MIR stack frame when not needed #57351

oli-obk commented Jan 5, 2019

oli-obk commented Jan 5, 2019

bors commented Jan 5, 2019

bors commented Jan 5, 2019

dotdash commented Jan 5, 2019

oli-obk commented Jan 6, 2019

oli-obk commented Jan 7, 2019

rust-timer commented Jan 7, 2019

rust-timer commented Jan 7, 2019

oli-obk commented Jan 7, 2019

RalfJung Jan 7, 2019

oli-obk Jan 8, 2019

RalfJung Jan 9, 2019 •

edited

Loading

RalfJung commented Jan 7, 2019 •

edited

Loading

Mark-Simulacrum commented Jan 7, 2019

RalfJung commented Jan 9, 2019

bors commented Jan 9, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

RalfJung commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

RalfJung commented Jan 10, 2019

pietroalbini commented Jan 10, 2019

Don't actually create a full MIR stack frame when not needed #57351

Don't actually create a full MIR stack frame when not needed #57351

Conversation

oli-obk commented Jan 5, 2019

oli-obk commented Jan 5, 2019

bors commented Jan 5, 2019

bors commented Jan 5, 2019

dotdash commented Jan 5, 2019

oli-obk commented Jan 6, 2019

oli-obk commented Jan 7, 2019

rust-timer commented Jan 7, 2019

rust-timer commented Jan 7, 2019

oli-obk commented Jan 7, 2019

RalfJung Jan 7, 2019

Choose a reason for hiding this comment

oli-obk Jan 8, 2019

Choose a reason for hiding this comment

RalfJung Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Jan 7, 2019 • edited Loading

Mark-Simulacrum commented Jan 7, 2019

RalfJung commented Jan 9, 2019

bors commented Jan 9, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

RalfJung commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

RalfJung commented Jan 10, 2019

pietroalbini commented Jan 10, 2019

RalfJung Jan 9, 2019 •

edited

Loading

RalfJung commented Jan 7, 2019 •

edited

Loading