Use different datastructure for MIRI relocations by michaelwoerister · Pull Request #50866 · rust-lang/rust

michaelwoerister · 2018-05-18T16:13:50Z

This PR makes relocations in MIRI used a sorted vector instead of a BTreeMap which should make a few common operations more efficient. Let's see if that's true.

r? @oli-obk

michaelwoerister · 2018-05-18T16:14:21Z

Before reviewing, let's check if that actually makes things faster.

@bors try

bors · 2018-05-18T16:14:33Z

⌛ Trying commit 34b4f03 with merge cff7e0f...

@oli-obk

[WIP] Use different datastructure for MIRI relocations This PR makes relocations in MIRI used a sorted vector instead of a `BTreeMap` which should make a few common operations more efficient. Let's see if that's true. r? @oli-obk

oli-obk · 2018-05-18T17:21:37Z

General question: do we have any way of profiling rustc? Like with cachegrind?

bors · 2018-05-18T17:52:38Z

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

michaelwoerister · 2018-05-18T18:40:52Z

@Mark-Simulacrum, could I get a perf run for this, please?

Mark-Simulacrum · 2018-05-19T15:46:10Z

@oli-obk The perf collector supports local benchmarking with a variety of "profilers" https://github.com/rust-lang-nursery/rustc-perf/tree/master/collector#profiling-local-builds.

Perf for cff7e0f queued.

ishitatsuyuki · 2018-05-20T04:08:26Z

@oli-obk @nnethercote is using cachegrind for analysing the hot spots.

nnethercote · 2018-05-20T04:50:29Z

See https://github.com/rust-lang-nursery/rustc-perf/tree/master/collector#profiling-local-builds for info on profiling rustc.

kennytm · 2018-05-20T15:54:43Z

Perf result: http://perf.rust-lang.org/compare.html?start=df40e61382a2cba0be621fdabb9971ce3475e9a7&end=cff7e0fa166ed63dd9f50276bff6c6314da33d88&stat=instructions%3Au

Generally faster except NLL.

nnethercote · 2018-05-20T22:36:06Z

Note that style-servo "clean incremental" builds are highly variable, and so those improvements should be ignored. So it's a slight improvement on coercions and inflate, and a slight regression on clap-rs.

oli-obk · 2018-05-20T22:44:43Z

This seems like a lot of complexity for slight gains. Not sure if it's worth it?

michaelwoerister · 2018-05-21T10:15:42Z

This seems like a lot of complexity for slight gains. Not sure if it's worth it?

The complexity is in a general purpose data structures that we might want to use elsewhere too instead of BTreeMap, so I think it's fine.

michaelwoerister · 2018-05-21T10:17:06Z

I'm not sure how trustworthy the NLL benchmarks are.

kennytm · 2018-05-21T10:43:51Z

Given that the NLL algorithm is still in development and would be improved from other fronts, I believe we could disregard the minor NLL slowdown.

oli-obk

oops, forgot to press the "Submit review" button

oli-obk · 2018-05-21T12:04:36Z

src/librustc_data_structures/sorted_map.rs

+use std::mem;
+use std::ops::{RangeBounds, Bound, Index, IndexMut};
+
+#[derive(Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]


Default would probably be a good derive to have here, too

oli-obk · 2018-05-21T12:05:36Z

src/librustc_data_structures/sorted_map.rs

+use std::ops::{RangeBounds, Bound, Index, IndexMut};
+
+#[derive(Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
+pub struct SortedMap<K: Ord, V> {


Add some docs explaining the type

oli-obk · 2018-05-21T12:05:38Z

src/librustc_data_structures/sorted_map.rs

+        }
+    }
+
+    // It is up to the caller to make sure that the elements are sorted by key


Make this a doc comment

oli-obk · 2018-05-21T12:08:46Z

src/librustc_data_structures/sorted_map.rs

+
+    #[inline]
+    pub fn insert(&mut self, key: K, mut value: V) -> Option<V> {
+        let index = self.data.binary_search_by(|&(ref x, _)| x.cmp(&key));


use binary_search_by_key (also in the other functions further down)

Since we can't copy the key in the general case, we'd have to sort by &K which results in:

let index = self.data.binary_search_by_key(&&key, |&(ref x, _)| x);

I'm not sure that's an improvement. I can wrap the lookup in a helper method though. That should make things nicer.

oli-obk · 2018-05-21T12:14:21Z

src/librustc_data_structures/sorted_map.rs

+        self.data.iter_mut().map(|&mut (ref mut k, _)| k).for_each(f);
+    }
+
+    // It is up to the caller to make sure that the elements are sorted by key


comment -> doc comment

oli-obk · 2018-05-21T17:21:52Z

r=me with a rebase

bors · 2018-05-22T13:04:02Z

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

…orting order which lookup relies on.

michaelwoerister · 2018-05-22T14:59:11Z

@bors r=oli-obk

Thanks for the review, @oli-obk!

bors · 2018-05-22T14:59:13Z

📌 Commit 95fac99 has been approved by oli-obk

bors · 2018-05-23T12:24:55Z

⌛ Testing commit 95fac99 with merge 1962a70...

@oli-obk

Use different datastructure for MIRI relocations This PR makes relocations in MIRI used a sorted vector instead of a `BTreeMap` which should make a few common operations more efficient. Let's see if that's true. r? @oli-obk

bors · 2018-05-23T14:26:27Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 1962a70 to master...

eddyb · 2018-05-23T14:44:39Z

src/librustc_data_structures/sorted_map.rs

+    /// It is up to the caller to make sure that the elements are sorted by key
+    /// and that there are no duplicates.
+    #[inline]
+    pub fn insert_presorted(&mut self, mut elements: Vec<(K, V)>) {


Couldn't this take a SortedMap?

It could, yes.

oli-obk · 2018-05-29T21:37:36Z

src/librustc_mir/interpret/memory.rs

-        for k in keys {
-            alloc.relocations.remove(&k);
-        }
+        alloc.relocations.remove_range(first ..= last);


I don't think this is doing what it's supposed to be doing. Writing to the first element of a (&(), &())

00 00 00 00 00 00 00 00 |-----4----||-----5-----|

will result in

00 00 00 00 00 00 00 00 |-----9----|

I have a fix incoming

rust-highfive assigned oli-obk May 18, 2018

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

kennytm added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 18, 2018

kennytm removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 20, 2018

oli-obk reviewed May 21, 2018

View reviewed changes

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2018

michaelwoerister force-pushed the relocations-in-vec branch from 027d376 to dcf3c99 Compare May 22, 2018 08:04

michaelwoerister added 5 commits May 22, 2018 16:36

Add SortedMap to rustc_data_structures.

879eb97

Use SortedMap instead of BTreeMap for relocations in MIRI.

3ed23a4

Remove benchmarks from SortedMap.

eaa796c

Remove SortedMap::iter_mut() since that allows to break the element s…

e850d78

…orting order which lookup relies on.

Cleanup SortedMap by wrapping element lookup in a method.

4bedc31

Add some doc comments to SortedMap.

95fac99

michaelwoerister force-pushed the relocations-in-vec branch from dcf3c99 to 95fac99 Compare May 22, 2018 14:54

michaelwoerister changed the title ~~[WIP] Use different datastructure for MIRI relocations~~ Use different datastructure for MIRI relocations May 22, 2018

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

bors merged commit 95fac99 into rust-lang:master May 23, 2018

This was referenced May 23, 2018

Make AllocId decoding thread-safe #50957

Closed

Miri api refactor #50967

Merged

eddyb reviewed May 23, 2018

View reviewed changes

oli-obk reviewed May 29, 2018

View reviewed changes

Uh oh!

Comments

Conversation

michaelwoerister commented May 18, 2018

Uh oh!

michaelwoerister commented May 18, 2018

Uh oh!

bors commented May 18, 2018

Uh oh!

oli-obk commented May 18, 2018

Uh oh!

bors commented May 18, 2018

Uh oh!

michaelwoerister commented May 18, 2018

Uh oh!

Mark-Simulacrum commented May 19, 2018

Uh oh!

ishitatsuyuki commented May 20, 2018

Uh oh!

nnethercote commented May 20, 2018

Uh oh!

kennytm commented May 20, 2018

Uh oh!

nnethercote commented May 20, 2018

Uh oh!

oli-obk commented May 20, 2018

Uh oh!

michaelwoerister commented May 21, 2018

Uh oh!

michaelwoerister commented May 21, 2018

Uh oh!

kennytm commented May 21, 2018

Uh oh!

oli-obk left a comment

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!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented May 21, 2018

Uh oh!

bors commented May 22, 2018

Uh oh!

michaelwoerister commented May 22, 2018

Uh oh!

bors commented May 22, 2018

Uh oh!

bors commented May 23, 2018

Uh oh!

bors commented May 23, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants