Remove slow HashSet during miri stack frame creation#49274

oli-obk · 2018-03-22T14:16:39Z

probably has a major impact on #48846

cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.

michaelwoerister

Looks good to me!

If this is something that is also called for trivial constants, I would suggest adding a fast path for that case, e.g.:

if num_locals == 0 {
    vec![]
} else {
    // scan blocks and statements
}

michaelwoerister · 2018-03-22T15:29:17Z

r=me with the optimization implemented (if it makes sense).

oli-obk · 2018-03-22T15:39:24Z

Even trivial constants have locals. But you're right, we can add a fast path: Constants and statics don't contain any StorageDead or StorageLive statements, so we don't need to run this loop at all, irrelevant of the complexity of the constant.

oli-obk · 2018-03-22T16:00:44Z

@bors r=michaelwoerister

bors · 2018-03-22T16:00:45Z

📌 Commit 9fa14e4 has been approved by michaelwoerister

eddyb · 2018-03-22T17:25:12Z

src/librustc_mir/interpret/eval_context.rs

-                let local = mir::Local::new(i + 1);
-                if !annotated_locals.contains(&local) {
-                    locals[i] = Some(Value::ByVal(PrimVal::Undef));
+        let mut locals = vec![Some(Value::ByVal(PrimVal::Undef)); num_locals];


Please use IndexVec here.

The reason we didn't use that is theat we don't actually use the return pointer local at all.

Fixed, the waste of one space is massively outweighted by the many .index() - 1 calls we had.

Are you sure? Now you have to allocate something on the heap even if it's never used.

For truly trivial constants (const FOO: usize = 42;) that is true, but even const FOO: usize = 40 + 2; will create that heap alloc.

I added a fast path for trivial constants

eddyb · 2018-03-22T17:27:16Z

src/librustc_mir/interpret/eval_context.rs

-        fn collect_storage_annotations<'mir, 'tcx>(mir: &'mir mir::Mir<'tcx>) -> HashSet<mir::Local> {
-            use rustc::mir::StatementKind::*;
-
-            let mut set = HashSet::new();


Would be a good idea to check for Hash{Map,Set} used by miri code: if it needs to be a hash map/set, it should be FxHash{Map,Set} instead.

Done, found a few in interpret::memory

oli-obk · 2018-03-22T21:12:26Z

@bors r-

eddyb · 2018-03-23T11:30:20Z

src/librustc_mir/interpret/eval_context.rs

-        self.locals[local.index() - 1] = None;
+        let old = self.locals[local];
+        self.locals[local] = None;
        return Ok(old);


This is just Ok(self.locals[local].take())

oli-obk · 2018-03-23T11:45:48Z

@bors r=michaelwoerister,eddyb

bors · 2018-03-23T11:45:48Z

📌 Commit f9019ae has been approved by michaelwoerister,eddyb

@michaelwoerister

…er,eddyb Remove slow HashSet during miri stack frame creation fixes rust-lang#49237 probably has a major impact on rust-lang#48846 r? @michaelwoerister cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.

@michaelwoerister

…er,eddyb Remove slow HashSet during miri stack frame creation fixes rust-lang#49237 probably has a major impact on rust-lang#48846 r? @michaelwoerister cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.

@michaelwoerister

…er,eddyb Remove slow HashSet during miri stack frame creation fixes rust-lang#49237 probably has a major impact on rust-lang#48846 r? @michaelwoerister cc @eddyb I know you kept telling me to use vectors instead of hash containers... Now I know why.

Rollup of 21 pull requests - Successful merges: #49046, #49076, #49089, #49120, #49121, #49122, #49162, #49193, #49194, #49203, #49229, #49235, #49254, #49268, #49273, #49274, #49290, #49312, #49314, #49318, #49299

Remove slow HashSet during miri stack frame creation

9839e5f

rust-highfive assigned michaelwoerister Mar 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2018

michaelwoerister reviewed Mar 22, 2018

View reviewed changes

Skip checking for Storage* statements in constants/statics

9fa14e4

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 Mar 22, 2018

eddyb suggested changes Mar 22, 2018

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 22, 2018

oli-obk added 3 commits March 23, 2018 08:31

Vec<_> -> IndexVec<Local, _>

bf8e4f2

Replace uses of Hash(Map|Set) with FxHash(Map|Set) in miri

b18b776

Don't allocate a local array at all if there are no locals

4ea4dd2

eddyb reviewed Mar 23, 2018

View reviewed changes

Simplify local accessors

f9019ae

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2018

alexcrichton mentioned this pull request Mar 23, 2018

Rollup of 20 pull requests #49317

Closed

kennytm mentioned this pull request Mar 24, 2018

Rollup of 21 pull requests #49325

Closed

kennytm mentioned this pull request Mar 24, 2018

Rollup of 21 pull requests #49337

Merged

bors merged commit f9019ae into rust-lang:master Mar 24, 2018

oli-obk deleted the slow_miri branch June 15, 2020 15:26

Uh oh!

Conversation

oli-obk commented Mar 22, 2018

Uh oh!

michaelwoerister left a comment

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Mar 22, 2018

Uh oh!

oli-obk commented Mar 22, 2018

Uh oh!

oli-obk commented Mar 22, 2018

Uh oh!

bors commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Mar 23, 2018

Uh oh!

bors commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oli-obk Mar 23, 2018 •

edited

Loading