Make const decoding thread-safe. #51060

michaelwoerister · 2018-05-25T15:32:39Z

This is an alternative to #50957. It's a proof of concept (e.g. it doesn't adapt metadata decoding, just the incr. comp. cache) but I think it turned out nice. It's rather simple and does not require passing around a bunch of weird closures, like we currently do.

If you (@Zoxc & @oli-obk) think this approach is good then I'm happy to finish and clean this up.

Note: The current version just spins when it encounters an in-progress decoding. I don't have a strong preference for this approach. Decoding concurrently is equally fine by me (or maybe even better because it doesn't require poisoning).

r? @Zoxc

bors · 2018-05-25T16:19:54Z

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

oli-obk

I like it!

oli-obk · 2018-05-25T17:17:45Z

src/librustc/mir/interpret/mod.rs

+                        // Don't recurse.
+                        return Ok(alloc_id);
+                    } else {
+                        // Just spin. We could also decode concurrently.


ideally we'd turn this into a work stealing algo for allocations with loads of relocations... someday

Yes, with Rayon it might not even be that hard.

oli-obk · 2018-05-25T17:21:12Z

src/librustc/mir/interpret/mod.rs

+enum State {
+    Empty,
+    InProgress(DecodingSessionId, AllocId),
+    Done(AllocId),


If you made this store the Result instead of just the Ok value, you can get rid of the panicking and poisoning

I'll look into it.

Zoxc · 2018-05-26T09:41:51Z

src/librustc/mir/interpret/mod.rs

+
+        match alloc {
+            Ok(alloc) => {
+                decoder.tcx().alloc_map.lock().set_id_alloc(alloc_id, alloc);


This should use the set_id_same_memory function I introduced in my PR. AllocKind::Fn and AllocKind::Static should still use create_fn_alloc and intern_static.

Can instance decoding potentially be expensive? Or DefId decoding?

This should use the set_id_same_memory function I introduced in my PR.

With the spinning solution (which again, I don't necessarily prefer) setting things in the alloc_map is not racy, so asserting that no key existed should be correct (and avoids doing a comparison).

Can instance decoding potentially be expensive? Or DefId decoding?

It's not free. I think we should cache statics and fns too, if it doesn't complicate the algorithm.

Yeah, you could just have a set_id_memory which just asserts that there was no existing entry.

It does that already, if I'm not mistaken.

Your accepts AllocType, which is incorrect, since AllocType::Function and AllocType::Static are interned. It should only accept allocations.

I'll look into that. Thanks for the hint!

michaelwoerister · 2018-05-28T12:09:09Z

I think, I'll go with the set_id_same_memory() approach for now. It's simpler.

michaelwoerister · 2018-05-28T12:18:40Z

Oh wait, it's not really simpler. Now we have to keep track of multiple decoding sessions...

michaelwoerister · 2018-05-28T15:55:54Z

Huh, it looks like we create a new interpret_alloc_cache and interpret_alloc_index every time something is decoded from metadata. That doesn't sound right :D

Zoxc · 2018-05-28T16:20:02Z

src/librustc/mir/interpret/mod.rs

+                            Some(alloc_id)
+                        },
+                        AllocKind::Fn | AllocKind::Static => {
+                            // Fns and statics can be cyclic and their AllocId


How can they be cyclic?

whoops, that should say "cannot be cyclic"

rust-highfive · 2018-05-28T17:22:48Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:50:20] ................................................................i...................................
[00:50:25] ....................................................................................................
[00:50:31] ....................................................................................................
[00:50:38] .............................................................................................i......
[00:50:41] ...........iiiiiiiii...................................................
[00:50:41] 
[00:50:41] travis_fold:start:test_ui_nll
travis_time:start:test_ui_nll
Check compiletest suite=ui mode=ui compare_mode=nll (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
[00:51:30] ................................................................i...................................
[00:51:34] ....................................................................................................
[00:51:40] ....................................................................................................
[00:51:46] .............................................................................................i......
[00:51:48] ...........iiiiiiiii...................................................
[00:51:48] 
[00:51:48]  finished in 67.856
[00:51:48] travis_fold:end:test_ui_nll

---
[01:27:22]    Compiling rustc_data_structures v0.0.0 (file:///checkout/src/librustc_data_structures)
[01:27:25] error[E0308]: mismatched types
[01:27:25]    --> librustc_data_structures/tiny_list.rs:177:36
[01:27:25]     |
[01:27:25] 177 |             assert!(!list.contains(i));
[01:27:25]     |                                    |
[01:27:25]     |                                    expected &u32, found u32
[01:27:25]     |                                    help: consider borrowing here: `&i`
[01:27:25]     |
---
[01:27:25] 
[01:27:25] To learn more, run the command again with --verbose.
[01:27:25] 
[01:27:25] 
[01:27:25] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "-p" "rustc_data_structures" "--" "--quiet"
[01:27:25] 
[01:27:25] 
[01:27:25] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:27:25] Build completed unsuccessfully in 0:39:32
[01:27:25] Build completed unsuccessfully in 0:39:32
[01:27:25] make: *** [check] Error 1
[01:27:25] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:06d52276
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2018-05-29T03:29:59Z

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

michaelwoerister · 2018-05-29T07:42:36Z

src/librustc/mir/interpret/mod.rs

+
+pub struct AllocDecodingState {
+    // For each AllocId we keep track of which decoding state it's currently in.
+    decoding_state: Vec<Mutex<State>>,


Note that I changed this from Mutex<Vec<_>> to Vec<Mutex<_>>. In theory this should reduce contention but it might be overkill.

michaelwoerister · 2018-05-29T08:54:29Z

Let's see if this changes performance. The metadata caching issue is fixed but the index of allocations is loaded eagerly now.

@bors try

bors · 2018-05-29T08:54:40Z

⌛ Trying commit 233c00c with merge cd7eecf...

@Zoxc

WIP: Make const decoding thread-safe. This is an alternative to #50957. It's a proof of concept (e.g. it doesn't adapt metadata decoding, just the incr. comp. cache) but I think it turned out nice. It's rather simple and does not require passing around a bunch of weird closures, like we currently do. If you (@Zoxc & @oli-obk) think this approach is good then I'm happy to finish and clean this up. Note: The current version just spins when it encounters an in-progress decoding. I don't have a strong preference for this approach. Decoding concurrently is equally fine by me (or maybe even better because it doesn't require poisoning). r? @Zoxc

michaelwoerister · 2018-05-29T08:55:19Z

Otherwise this should be ready for review, @Zoxc.

bors · 2018-05-29T10:59:55Z

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

michaelwoerister · 2018-05-29T11:03:58Z

@Mark-Simulacrum, could we get a perf run here please?

michaelwoerister · 2018-05-29T12:29:55Z

@Zoxc & @oli-obk, do you think we should use spinning instead of racing? I don't really care, it's your call.

oli-obk · 2018-05-29T13:31:26Z

No opinion as long as we always have the plan to use work stealing at some point

michaelwoerister · 2018-05-30T14:24:59Z

Here is the perf link: http://perf.rust-lang.org/compare.html?start=4f6d9bf2099c83f365bb0b3cf02091b4737c2052&end=cd7eecf54f05b3c5e0cf3797b5764cdb0bcd3972&stat=instructions%3Au

But I don't think the perf run has been done yet (cc @Mark-Simulacrum)

Mark-Simulacrum · 2018-05-30T15:28:44Z

I've queued per

michaelwoerister · 2018-05-30T15:41:46Z

Thanks, @Mark-Simulacrum!

michaelwoerister · 2018-05-30T19:32:17Z

Performance looks OK.

Zoxc · 2018-05-31T14:59:06Z

@bors r+

bors · 2018-05-31T14:59:07Z

📌 Commit 233c00c has been approved by Zoxc

michaelwoerister · 2018-05-31T15:04:36Z

Thanks for the review!

bors · 2018-05-31T19:53:24Z

🔒 Merge conflict

michaelwoerister · 2018-06-01T07:33:12Z

Rebased.

@bors r=Zoxc

bors · 2018-06-01T07:33:14Z

📌 Commit 12e8359 has been approved by Zoxc

bors · 2018-06-01T08:46:44Z

⌛ Testing commit 12e8359 with merge 4412902...

@Zoxc

Make const decoding thread-safe. This is an alternative to #50957. It's a proof of concept (e.g. it doesn't adapt metadata decoding, just the incr. comp. cache) but I think it turned out nice. It's rather simple and does not require passing around a bunch of weird closures, like we currently do. If you (@Zoxc & @oli-obk) think this approach is good then I'm happy to finish and clean this up. Note: The current version just spins when it encounters an in-progress decoding. I don't have a strong preference for this approach. Decoding concurrently is equally fine by me (or maybe even better because it doesn't require poisoning). r? @Zoxc

bors · 2018-06-01T10:48:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Zoxc
Pushing 4412902 to master...

rust-highfive assigned Zoxc May 25, 2018

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

oli-obk reviewed May 25, 2018

View reviewed changes

Zoxc reviewed May 26, 2018

View reviewed changes

michaelwoerister force-pushed the thread-safe-consts branch 2 times, most recently from 482bbf3 to ee16739 Compare May 28, 2018 15:53

Zoxc reviewed May 28, 2018

View reviewed changes

michaelwoerister force-pushed the thread-safe-consts branch from ee16739 to 1afcaf6 Compare May 28, 2018 18:05

michaelwoerister commented May 29, 2018

View reviewed changes

michaelwoerister force-pushed the thread-safe-consts branch from 1afcaf6 to 233c00c Compare May 29, 2018 08:52

michaelwoerister changed the title ~~WIP: Make const decoding thread-safe.~~ Make const decoding thread-safe. May 29, 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-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2018

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

michaelwoerister added 4 commits June 1, 2018 09:32

Add TinyList data structure.

f9f90ed

Make const decoding from the incremental cache thread-safe.

24dfcbe

Make metadata decoding use AllocDecodingState/Session.

22f6163

Remove outdated AllocId decoding function.

12e8359

michaelwoerister force-pushed the thread-safe-consts branch from 233c00c to 12e8359 Compare June 1, 2018 07:32

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 Jun 1, 2018

bors merged commit 12e8359 into rust-lang:master Jun 1, 2018

bors mentioned this pull request Jun 1, 2018

Make AllocId decoding thread-safe #50957

Closed

saethlin mentioned this pull request Jul 7, 2024

ICE: "assertion failed: *old == value" in optimized + incremental builds #126741

Closed

Make const decoding thread-safe. #51060

Make const decoding thread-safe. #51060

Conversation

michaelwoerister commented May 25, 2018

bors commented May 25, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented May 28, 2018

michaelwoerister commented May 28, 2018

michaelwoerister commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented May 28, 2018

bors commented May 29, 2018

Choose a reason for hiding this comment

michaelwoerister commented May 29, 2018

bors commented May 29, 2018

michaelwoerister commented May 29, 2018

bors commented May 29, 2018

michaelwoerister commented May 29, 2018

michaelwoerister commented May 29, 2018

oli-obk commented May 29, 2018

michaelwoerister commented May 30, 2018

Mark-Simulacrum commented May 30, 2018

michaelwoerister commented May 30, 2018

michaelwoerister commented May 30, 2018

Zoxc commented May 31, 2018

bors commented May 31, 2018

michaelwoerister commented May 31, 2018

bors commented May 31, 2018

michaelwoerister commented Jun 1, 2018

bors commented Jun 1, 2018

bors commented Jun 1, 2018

bors commented Jun 1, 2018