Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SelectionCache and EvaluationCache thread-safe #49834

Merged
merged 1 commit into from
May 9, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 10, 2018

Split out from #49558

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2018
@TimNN
Copy link
Contributor

TimNN commented Apr 10, 2018

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.
Resolving deltas: 100% (613496/613496), completed with 4872 local objects.
---
[00:00:41] configure: rust.quiet-tests     := True
---
[00:37:20] thread 'main' panicked at 'assertion failed: *old == value', librustc_data_structures/sync.rs:236:42

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)

// This may overwrite the cache with the same value
tcx.selection_cache
.hashmap.borrow_mut()
.insert(trait_ref, WithDepNode::new(dep_node, candidate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comment above, should this also use insert_same()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inserted type doesn't implement Eq.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2018

📌 Commit 9d48917 has been approved by nikomatsakis

@bors 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 Apr 11, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 11, 2018

This doesn't work since insert_evaluation_cache overwrites values.

@bors r-

@bors 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 Apr 11, 2018
@bors
Copy link
Contributor

bors commented Apr 12, 2018

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

@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 16, 2018
@rust-highfive
Copy link
Collaborator

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:01:07] configure: rust.quiet-tests     := True
---
[00:42:45] thread 'main' panicked at 'assertion failed: *old == value', librustc_data_structures/sync.rs:241:42
---
[00:43:33] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "doc" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--no-deps" "-p" "alloc" "-p" "core" "-p" "std" "-p" "std_unicode"
[00:43:33] expected success, got: exit code: 101
[00:43:33]
[00:43:33]
[00:43:33] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap doc
[00:43:33] Build completed unsuccessfully in 0:05:47
[00:43:33] make: *** [all] Error 1
[00:43:33] Makefile:28: recipe for target 'all' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:2705b080:start=1523980841568033483,finish=1523980841575288725,duration=7255242
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:294834ea
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:294834ea:start=1523980841581578671,finish=1523980841588508966,duration=6930295
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09c17180
$ dmesg | grep -i kill
[   10.544864] init: failsafe main process (1093) killed by TERM signal

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)

@nikomatsakis
Copy link
Contributor

@Zoxc to be honest I didn't think too hard here-- and I still haven't. =) Can you spell out the problem, and do you have any ideas for how to solve?

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2018

@nikomatsakis I though maps in SelectionCache and EvaluationCache would have values which are deterministically based on their keys. That is not true for EvaluationCache at least, since it will change the value of aty::PolyTraitRef<'tcx> key to a different WithDepNode<EvaluationResult>. I'm wondering if this is expected behavior, and if so, how to make these caches thread-safe?

@shepmaster
Copy link
Member

Ping from triage, @Zoxc and @nikomatsakis ! Will you have time to make progress on this in the near future?

@rust-highfive
Copy link
Collaborator

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:37:03] Documenting book redirect pages (x86_64-unknown-linux-gnu)
[00:37:04] Documenting stage2 std (x86_64-unknown-linux-gnu)
[00:37:04]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[00:37:04]  Documenting core v0.0.0 (file:///checkout/src/libcore)
[00:37:17] overwriting key/trait_ref: Binder(<isize as marker::Freeze>) with EvaluationResult WithDepNode { dep_node: 150890, cached_value: EvaluatedToOk }
[00:37:17]  old value: WithDepNode { dep_node: 150889, cached_value: EvaluatedToOk }
[00:37:17] thread 'main' panicked at 'assertion failed: *old == value', librustc_data_structures/sync.rs:241:42
[00:37:17] 
[00:37:17] error: internal compiler error: unexpected panic
[00:37:17] 
[00:37:17] 
[00:37:17] note: the compiler unexpectedly panicked. this is a bug.
[00:37:17] 
[00:37:17] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:37:17] 
[00:37:17] note: rustc 1.27.0-dev running on x86_64-unknown-linux-gnu
[00:37:17] 
[00:37:17] note: compiler flags: -Z force-unstable-if-unmarked -C debug-assertions=off -C overflow-checks=on -C incremental -C prefer-dynamic -C debug-assertions=y -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib
[00:37:17] 
[00:37:17] note: some of the compiler flags provided by cargo are hidden
[00:37:17] error: Could not compile `core`.
[00:37:17] warning: build failed, waiting for other jobs to finish...
[00:37:49] warning: [1] cannot be resolved, ignoring it...
[00:37:49] 
---
[00:37:51] 
[00:37:55] error: build failed
[00:37:55] 
[00:37:55] 
[00:37:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "doc" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--no-deps" "-p" "alloc" "-p" "core" "-p" "std" "-p" "std_unicode"
[00:37:55] 
[00:37:55] 
[00:37:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap doc
[00:37:55] Build completed unsuccessfully in 0:05:15
[00:37:55] Build completed unsuccessfully in 0:05:15
[00:37:55] Makefile:28: recipe for target 'all' failed
[00:37:55] make: *** [all] Error 1
2174820 ./obj
2174788 ./obj/build
1566936 ./obj/build/x86_64-unknown-linux-gnu
725072 ./src
---
149120 ./src/llvm-emscripten/test
144660 ./obj/build/bootstrap/debug/incremental
128084 ./obj/build/x86_64-unknown-linux-gnu/stage1-std
123692 ./obj/build/bootstrap/debug/incremental/bootstrap-1wl4zjaz72e5d
123688 ./obj/build/bootstrap/debug/incremental/bootstrap-1wl4zjaz72e5d/s-f0msmdn1je-1ia77dq-6gvofocobazn
121820 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
105360 ./obj/build/x86_64-unknown-linux-gnu/crate-docs
103452 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu
103448 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release

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)

@shepmaster
Copy link
Member

Ping from triage, @Zoxc ! You have test failures.

@Zoxc
Copy link
Contributor Author

Zoxc commented May 7, 2018

I've created #50507 for the bug which overwrites the cache and added a fixme to use insert_same when that is fixed.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 9, 2018

📌 Commit f741ee1 has been approved by nikomatsakis

@bors 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 9, 2018
@bors
Copy link
Contributor

bors commented May 9, 2018

⌛ Testing commit f741ee1 with merge e5f80f2...

bors added a commit that referenced this pull request May 9, 2018
Make SelectionCache and EvaluationCache thread-safe

Split out from #49558

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 9, 2018

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

@bors bors merged commit f741ee1 into rust-lang:master May 9, 2018
@Zoxc Zoxc deleted the sync-trait-cache branch May 9, 2018 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants