Replace MemoryExtra by Memory in intptrcast methods #62003

pvdrz · 2019-06-20T19:09:17Z

RalfJung · 2019-06-20T19:12:08Z

Can you change tag_allocation and tag_static_base_pointer in the same way, for consistency?

RalfJung · 2019-06-20T19:12:39Z

Also, please prepare a Miri PR that fixes compiling Miri with these changes. It should not do anything else, so we can land it quickly once this one here lands.

pvdrz · 2019-06-20T19:17:26Z

Sure, I'll do the change in rustc and then do the respective miri PR

oli-obk · 2019-06-21T06:37:12Z

@bors r+

bors · 2019-06-21T06:37:13Z

📌 Commit e152c38 has been approved by oli-obk

bors · 2019-06-21T06:37:24Z

⌛ Testing commit e152c38 with merge 56a12b2...

@RalfJung

Replace MemoryExtra by Memory in intptrcast methods r? @RalfJung @oli-obk

bors · 2019-06-21T09:19:37Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 56a12b2 to master...

rust-highfive · 2019-06-21T09:20:38Z

📣 Toolstate changed by #62003!

Tested on commit 56a12b2.
Direct link to PR: #62003

💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

@oli-obk

Tested on commit rust-lang/rust@56a12b2. Direct link to PR: <rust-lang/rust#62003> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

RalfJung · 2019-06-21T10:58:42Z

@rust-lang/infra no issue was created... did I screw up in #61938 ?

RalfJung · 2019-06-21T11:00:08Z

The logs seem reasonable?

[01:41:13] Cloning into 'rust-toolstate'...
[01:41:14] The state of "miri" has changed from "test-pass" to "build-fail"
[01:41:14] [master cd8ed9f] (linux CI update)
[01:41:14]  1 file changed, 1 insertion(+)
[01:41:17] To https://github.com/rust-lang-nursery/rust-toolstate.git
[01:41:17]    17c7197..cd8ed9f  master -> master

RalfJung · 2019-06-21T11:02:54Z

Oh yes, I did screw up... new is used inside the loop, not outside of it. Not sure how that would have this effect though, instead of showing an error.

And with Python's scoping rules, new should be the state of the last OS it looks at, so this should actually work.

RalfJung · 2019-06-21T11:16:56Z

Hm no, that log is created by a totally different script, at https://github.com/rust-lang/rust/blob/56a12b2ad058f22f1ef090713df15598525ba4a4/src/ci/docker/x86_64-gnu-tools/checkregression.py, called at

rust/src/ci/docker/x86_64-gnu-tools/checktools.sh

Line 101 in 56a12b2

    
           if python2.7 "$CHECK_NOT" "$OS" "$TOOLSTATE_FILE" "_data/latest.json" changed; then

RalfJung · 2019-06-21T11:39:27Z

(Continuing at #62023.)

publish_toolstate: don't use 'new' from inside the loop I think I made a mistake in rust-lang#61938 by using `new` outside the inner loop. This PR fixes that. However, no issue got created at all for rust-lang#62003 (comment), and I don't know why that is. The script should be [triggered by Traivs](https://github.com/rust-lang/rust/blob/56a12b2ad058f22f1ef090713df15598525ba4a4/.travis.yml#L240), and I can find no trace of that in [the build log](https://travis-ci.com/rust-lang/rust/jobs/209880042).

nnethercote · 2019-06-30T22:36:44Z

This caused a perf regression. Is that expected? Could the regression be avoided?

RalfJung · 2019-06-30T22:50:56Z

No that's not expected. In fact it seems really strange because the only instance of all of these methods that's used in rustc is basically doing nothing.

Maybe the default instances need to be marked inline(always)?

…-obk" This reverts commit 56a12b2, reversing changes made to dbec74f.

@nnethercote

[WIP] Debug perf regression from Miri Machine trait changes Maybe this fixes the perf regression that #62003 seemingly introduced? Cc @nnethercote

RalfJung · 2019-07-02T09:12:10Z

src/librustc_mir/interpret/memory.rs

@@ -416,11 +416,11 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
        id: AllocId,
    ) -> InterpResult<'tcx, &mut Allocation<M::PointerTag, M::AllocExtra>> {
        let tcx = self.tcx;
-        let memory_extra = &self.extra;
+        let alloc = Self::get_static_alloc(id, tcx, &self);


Oh, I had entirely missed this! This is not what we want. This is the reason for the perf regression. We don't want to ask for a static alloc if we don't absolutely need it. You turned the vast majority of the get_mut calls from one to two HashMap lookups.

This could also explain why MIRI_BACKTRACE=1 made everything so awfully slow when I used it for debugging on CI.

Oh I remember that change, that was because there was no way to pass self into the get_mut_or without having a mutable and an immutable reference at the same time.

Before doing the MemoryExtra -> Memory change, we passed just self.extra and get_mut_or was called from self.alloc_map so there was no borrowing issues.

Yeah, that's why I am reverting to just MemoryExtra now.

Next time you have to reorder code like that in a way that moves an operation out of a closure and makes it executed much more often than previously... please call that out when submitting the PR, so we can review that specifically. :) I should have seen this in the review, but it gets easier when you call out the "most interesting" bits.

Yeah sorry, that was a little careless from my part. Next time will do

Edit: I assume we could modify how get_mut_or works or adding a get_mut but I don't think it is worth it

@nnethercote

Fix perf regression from Miri Machine trait changes Maybe this fixes the perf regression that #62003 seemingly introduced? Cc @nnethercote

@nnethercote

Fix perf regression from Miri Machine trait changes Maybe this fixes the perf regression that rust-lang#62003 seemingly introduced? Cc @nnethercote

@nnethercote

Fix perf regression from Miri Machine trait changes Maybe this fixes the perf regression that rust-lang#62003 seemingly introduced? Cc @nnethercote

Replace MemoryExtra by Memory in intptrcast methods

9cc28d4

rust-highfive assigned RalfJung Jun 20, 2019

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

pvdrz added 2 commits June 20, 2019 16:52

Updated tag methods for consistency

752a1a4

Fix merge issues

e152c38

pvdrz mentioned this pull request Jun 21, 2019

Update tag methods to match Machine changes rust-lang/miri#778

Merged

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 Jun 21, 2019

bors added a commit that referenced this pull request Jun 21, 2019

Auto merge of #62003 - christianpoveda:master, r=oli-obk

56a12b2

Replace MemoryExtra by Memory in intptrcast methods r? @RalfJung @oli-obk

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2019

bors merged commit e152c38 into rust-lang:master Jun 21, 2019

RalfJung mentioned this pull request Jun 21, 2019

publish_toolstate: don't use 'new' from inside the loop #62023

Merged

kennytm mentioned this pull request Jun 21, 2019

no issue created when clippy toolstate breaks #61693

Closed

RalfJung mentioned this pull request Jul 1, 2019

Fix perf regression from Miri Machine trait changes #62264

Merged

RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 1, 2019

Revert "Auto merge of rust-lang#62003 - christianpoveda:master, r=oli…

269866b

…-obk" This reverts commit 56a12b2, reversing changes made to dbec74f.

bors added a commit that referenced this pull request Jul 1, 2019

Auto merge of #62264 - RalfJung:inline-forcing, r=<try>

32136d3

[WIP] Debug perf regression from Miri Machine trait changes Maybe this fixes the perf regression that #62003 seemingly introduced? Cc @nnethercote

RalfJung reviewed Jul 2, 2019

View reviewed changes

bors added a commit that referenced this pull request Jul 2, 2019

Auto merge of #62264 - RalfJung:inline-forcing, r=<try>

ed4ad9c

Fix perf regression from Miri Machine trait changes Maybe this fixes the perf regression that #62003 seemingly introduced? Cc @nnethercote

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace MemoryExtra by Memory in intptrcast methods #62003

Replace MemoryExtra by Memory in intptrcast methods #62003

pvdrz commented Jun 20, 2019

RalfJung commented Jun 20, 2019

RalfJung commented Jun 20, 2019

pvdrz commented Jun 20, 2019

oli-obk commented Jun 21, 2019

bors commented Jun 21, 2019

bors commented Jun 21, 2019

bors commented Jun 21, 2019

rust-highfive commented Jun 21, 2019

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019 •

edited

Loading

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019

nnethercote commented Jun 30, 2019

RalfJung commented Jun 30, 2019

RalfJung Jul 2, 2019

RalfJung Jul 2, 2019

pvdrz Jul 2, 2019 •

edited

Loading

RalfJung Jul 2, 2019

pvdrz Jul 2, 2019 •

edited

Loading

Replace MemoryExtra by Memory in intptrcast methods #62003

Replace MemoryExtra by Memory in intptrcast methods #62003

Conversation

pvdrz commented Jun 20, 2019

RalfJung commented Jun 20, 2019

RalfJung commented Jun 20, 2019

pvdrz commented Jun 20, 2019

oli-obk commented Jun 21, 2019

bors commented Jun 21, 2019

bors commented Jun 21, 2019

bors commented Jun 21, 2019

rust-highfive commented Jun 21, 2019

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019 • edited Loading

RalfJung commented Jun 21, 2019

RalfJung commented Jun 21, 2019

nnethercote commented Jun 30, 2019

RalfJung commented Jun 30, 2019

RalfJung Jul 2, 2019

Choose a reason for hiding this comment

RalfJung Jul 2, 2019

Choose a reason for hiding this comment

pvdrz Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

RalfJung Jul 2, 2019

Choose a reason for hiding this comment

pvdrz Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Jun 21, 2019 •

edited

Loading

pvdrz Jul 2, 2019 •

edited

Loading

pvdrz Jul 2, 2019 •

edited

Loading