Remove all reference to DepGraph::work_products #50558

whitfin · 2018-05-09T05:51:01Z

This is an attempt at fixing #50500. It will remove the work_products key from DepGraphData completely, in favour of just passing the relevant data around. I went in a little blindly; everything appears to work just fine but I'd appreciate any additional advice people.

I didn't want to remove too much of what was already there, so I kept the structure pretty much the same (aside from some naming tweaks) - if anyone has suggestions on how to streamline it a little better, happy to follow up.

r? @michaelwoerister

rust-highfive · 2018-05-09T05:51:11Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive · 2018-05-09T06:04:32Z

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:04:37] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:37] tidy error: /checkout/src/librustc_trans/back/write.rs:2247: line longer than 100 chars
[00:04:39] some tidy checks failed
[00:04:39] 
[00:04:39] 
[00:04:39] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:39] 
[00:04:39] 
[00:04:39] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:39] Build completed unsuccessfully in 0:01:51
[00:04:39] Build completed unsuccessfully in 0:01:51
[00:04:39] Makefile:79: recipe for target 'tidy' failed
[00:04:39] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:10c7cf94
$ 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)

michaelwoerister

Thanks, @whitfin! That's how I imagined the refactoring to look like.

I've never been very happy with the naming in this area, so changing them is a good idea. However, I think the names you chose make it less clear what the functions do. I left some suggestions for new names below.

michaelwoerister · 2018-05-09T08:52:35Z

src/librustc_incremental/persist/save.rs

@@ -55,22 +55,22 @@ pub fn save_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
    })
 }

-pub fn save_work_products(sess: &Session, dep_graph: &DepGraph) {
+pub fn save_work_products(sess: &Session,


This could be renamed to save_work_product_index.

michaelwoerister · 2018-05-09T08:53:25Z

src/librustc_incremental/persist/save.rs

@@ -234,10 +234,9 @@ fn encode_dep_graph(tcx: TyCtxt,
    Ok(())
 }

-fn encode_work_products(dep_graph: &DepGraph,
+fn encode_work_products(work_products: &FxHashMap<WorkProductId, WorkProduct>,


Rename to encode_work_product_index.

michaelwoerister · 2018-05-09T08:54:36Z

src/librustc_incremental/persist/work_product.rs

-                            cgu_name: &str,
-                            files: &[(WorkProductFileKind, PathBuf)]) {
-    debug!("save_trans_partition({:?},{:?})",
+pub fn create_trans_partition(sess: &Session,


Rename to copy_cgu_workproducts_to_incr_comp_cache_dir.

michaelwoerister · 2018-05-09T08:56:11Z

src/librustc_trans/back/write.rs

-fn copy_module_artifacts_into_incr_comp_cache(sess: &Session,
-                                              dep_graph: &DepGraph,
-                                              compiled_modules: &CompiledModules) {
+fn generate_module_artifacts(


Rename to copy_all_cgu_workproducts_to_incr_comp_cache_dir

whitfin · 2018-05-09T15:30:28Z

Hi @michaelwoerister, thank you for the naming tips! I honestly had no idea why the names were what they were, so my changes were mostly guesses :p I have updated using your suggestions!

michaelwoerister · 2018-05-09T15:44:28Z

Excellent, thanks for updating!

@bors r+

bors · 2018-05-09T15:44:29Z

📌 Commit 6d5ed01 has been approved by michaelwoerister

michaelwoerister · 2018-05-10T13:26:18Z

@bors rollup

Rollup of 18 pull requests Successful merges: - #49423 (Extend tests for RFC1598 (GAT)) - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)) - #50447 (Fix update-references for tests within subdirectories.) - #50514 (Pull in a wasm fix from LLVM upstream) - #50524 (Make DepGraph::previous_work_products immutable) - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.) - #50538 ( Make CrateNum allocation more thread-safe. ) - #50564 (Inline `Span` methods.) - #50565 (Use SmallVec for DepNodeIndex within dep_graph.) - #50569 (Allow for specifying a linker plugin for cross-language LTO) - #50572 (Clarify in the docs that `mul_add` is not always faster.) - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022)) - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`) - #50588 (Move "See also" disambiguation links for primitive types to top) - #50590 (Fix tuple struct field spans) - #50591 (Restore RawVec::reserve* documentation) - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize) - #50606 (Retry when downloading the Docker cache.) Failed merges: - #50161 (added missing implementation hint) - #50558 (Remove all reference to DepGraph::work_products)

whitfin · 2018-05-11T02:11:44Z

@michaelwoerister I had to update this for some changes on master after some merges; unsure if that means you have to re-approve?

rust-highfive · 2018-05-11T02:19:51Z

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.

    100% |████████████████████████████████| 61kB 8.3MB/s 
Collecting botocore==1.10.19 (from awscli)
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/34/05/1ffe77f2b8fb03a9223f3d0743cd38cded9491b074c65f3bb664ed4b7ac6/botocore-1.10.19-py2.py3-none-any.whl (4.2MB)
    0% |                                | 10kB 41.6MB/s eta 0:00:01
    0% |▏                               | 20kB 42.3MB/s eta 0:00:01
    0% |▎                               | 30kB 40.2MB/s eta 0:00:01
    0% |▎                               | 40kB 29.0MB/s eta 0:00:01
---
[00:05:36]    Compiling backtrace v0.3.6
[00:05:43]    Compiling rustc_errors v0.0.0 (file:///checkout/src/librustc_errors)
[00:07:14]    Compiling proc_macro v0.0.0 (file:///checkout/src/libproc_macro)
[00:07:31]    Compiling syntax_ext v0.0.0 (file:///checkout/src/libsyntax_ext)
[00:07:36] error: unused imports: `ReadGuard`, `RwLock`
[00:07:36]   --> librustc/dep_graph/graph.rs:16:40
[00:07:36]    |
[00:07:36] 16 | use rustc_data_structures::sync::{Lrc, RwLock, ReadGuard, Lock};
[00:07:36]    |                                        ^^^^^^  ^^^^^^^^^
[00:07:36]    = note: `-D unused-imports` implied by `-D warnings`
[00:07:36] 
ll] Error 1
ll] Error 1
[00:08:02] Makefile:28: recipe for target 'all' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0532c098
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri May 11 02:19:47 UTC 2018
185428 ./obj/build/cache/2018-04-24
149724 ./.git/modules
149720 ./.git/modules/src
149120 ./src/llvm-emscripten/test
148912 ./obj/build/bootstrap/debug/incremental
124332 ./obj/build/bootstrap/debug/incremental/bootstrap-182x3aewwy26b
124328 ./obj/build/bootstrap/debug/incremental/bootstrap-182x3aewwy26b/s-f0x8xr8x5k-aeeear-22v3gsapbxtg7
89800 ./src/llvm/test/CodeGen
83616 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
71468 ./.git/modules/src/tools
70944 ./obj/build/x86_64-unknown-linux-gnu/native

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)

michaelwoerister · 2018-05-11T11:17:30Z

Yes, I have to re-approve everytime something changes. A quick ping after rebasing is helpful, thanks!

Could you do a proper rebase though (e.g. git rebase upstream/master)? We don't allow merge commits (except for those that CI introduces).

whitfin · 2018-05-11T15:13:45Z

@michaelwoerister ok, updated (I think)! Let me know if I rebased properly; not entirely sure if I did the right thing :p

michaelwoerister · 2018-05-11T16:17:47Z

Looks good!

@bors r+

bors · 2018-05-11T16:17:48Z

📌 Commit 8402a58 has been approved by michaelwoerister

@michaelwoerister

…ster Remove all reference to DepGraph::work_products This is an attempt at fixing rust-lang#50500. It will remove the `work_products` key from `DepGraphData` completely, in favour of just passing the relevant data around. I went in a little blindly; everything appears to work just fine but I'd appreciate any additional advice people. I didn't want to remove too much of what was already there, so I kept the structure pretty much the same (aside from some naming tweaks) - if anyone has suggestions on how to streamline it a little better, happy to follow up. r? @michaelwoerister

@michaelwoerister

…ster Remove all reference to DepGraph::work_products This is an attempt at fixing rust-lang#50500. It will remove the `work_products` key from `DepGraphData` completely, in favour of just passing the relevant data around. I went in a little blindly; everything appears to work just fine but I'd appreciate any additional advice people. I didn't want to remove too much of what was already there, so I kept the structure pretty much the same (aside from some naming tweaks) - if anyone has suggestions on how to streamline it a little better, happy to follow up. r? @michaelwoerister

@michaelwoerister

…ster Remove all reference to DepGraph::work_products This is an attempt at fixing rust-lang#50500. It will remove the `work_products` key from `DepGraphData` completely, in favour of just passing the relevant data around. I went in a little blindly; everything appears to work just fine but I'd appreciate any additional advice people. I didn't want to remove too much of what was already there, so I kept the structure pretty much the same (aside from some naming tweaks) - if anyone has suggestions on how to streamline it a little better, happy to follow up. r? @michaelwoerister

Rollup of 13 pull requests Successful merges: - #50544 (Cleanup some dependencies) - #50545 (Made some functions in time module const) - #50550 (use fmt::Result where applicable) - #50558 (Remove all reference to DepGraph::work_products) - #50602 (Update canonicalize docs) - #50607 (Allocate Symbol strings from an arena) - #50613 (Migrate the toolstate update bot to rust-highfive) - #50624 (fs::write: Add example writing a &str) - #50634 (Do not silently truncate offsets for `read_at`/`write_at` on emscripten) - #50644 (AppVeyor: Read back trace from crash dump on failure.) - #50661 (Ignore non .rs files for tidy libcoretest) - #50663 (rustc: Allow an edition's feature on that edition) - #50667 (rustc: Only suggest deleting `extern crate` if it works) Failed merges:

rust-highfive assigned michaelwoerister May 9, 2018

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

michaelwoerister reviewed May 9, 2018

View reviewed changes

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 9, 2018

This was referenced May 10, 2018

Rollup of 19 pull requests #50608

Closed

Rollup of 18 pull requests #50611

Merged

whitfin added 5 commits May 11, 2018 08:09

Remove shared access to DepGraph::work_products

89a8f2c

Catch a bad reference in use clauses

d23cbc3

Neaten a couple of long signatures

5c83422

Update naming in line with PR comments

0a4fbe3

Update an old method name in debug logging

8402a58

whitfin force-pushed the issue-50500 branch from 35cdf66 to 8402a58 Compare May 11, 2018 15:12

alexcrichton mentioned this pull request May 11, 2018

Rollup of 10 pull requests #50666

Closed

kennytm mentioned this pull request May 12, 2018

Rollup of 11 pull requests #50681

Closed

Mark-Simulacrum mentioned this pull request May 12, 2018

Rollup of 13 pull requests #50686

Merged

bors merged commit 8402a58 into rust-lang:master May 12, 2018

whitfin deleted the issue-50500 branch May 12, 2018 23:01

michaelwoerister mentioned this pull request May 23, 2018

[parallel-queries] Refactor away DepGraph::work_products to avoid shared mutable state #50500

Closed

nikomatsakis mentioned this pull request Jul 4, 2018

Query Parallelization Tracking Issue #48685

Open

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all reference to DepGraph::work_products #50558

Remove all reference to DepGraph::work_products #50558

whitfin commented May 9, 2018

rust-highfive commented May 9, 2018

rust-highfive commented May 9, 2018

michaelwoerister left a comment

michaelwoerister May 9, 2018

michaelwoerister May 9, 2018

michaelwoerister May 9, 2018

michaelwoerister May 9, 2018

whitfin commented May 9, 2018

michaelwoerister commented May 9, 2018

bors commented May 9, 2018

michaelwoerister commented May 10, 2018

whitfin commented May 11, 2018

rust-highfive commented May 11, 2018

michaelwoerister commented May 11, 2018

whitfin commented May 11, 2018

michaelwoerister commented May 11, 2018

bors commented May 11, 2018

Remove all reference to DepGraph::work_products #50558

Remove all reference to DepGraph::work_products #50558

Conversation

whitfin commented May 9, 2018

rust-highfive commented May 9, 2018

rust-highfive commented May 9, 2018

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

whitfin commented May 9, 2018

michaelwoerister commented May 9, 2018

bors commented May 9, 2018

michaelwoerister commented May 10, 2018

whitfin commented May 11, 2018

rust-highfive commented May 11, 2018

michaelwoerister commented May 11, 2018

whitfin commented May 11, 2018

michaelwoerister commented May 11, 2018

bors commented May 11, 2018