Remove all reference to DepGraph::work_products by whitfin · Pull Request #50558 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

whitfin

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

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

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

Choose a reason for hiding this comment

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

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.

@@ -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,

Choose a reason for hiding this comment

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

This could be renamed to save_work_product_index.

@@ -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>,

Choose a reason for hiding this comment

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

Rename to encode_work_product_index.

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

Choose a reason for hiding this comment

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

Rename to copy_cgu_workproducts_to_incr_comp_cache_dir.

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

Choose a reason for hiding this comment

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

Rename to copy_all_cgu_workproducts_to_incr_comp_cache_dir

@whitfin

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

Excellent, thanks for updating!

@bors r+

@bors

📌 Commit 6d5ed01 has been approved by michaelwoerister

@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

May 9, 2018

@michaelwoerister

This was referenced

May 10, 2018

bors added a commit that referenced this pull request

May 10, 2018

@bors

Rollup of 18 pull requests

Successful merges:

Failed merges:

@whitfin

@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

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

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

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

@michaelwoerister

@bors

📌 Commit 8402a58 has been approved by michaelwoerister

alexcrichton added a commit to alexcrichton/rust that referenced this pull request

May 11, 2018

@alexcrichton

…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

kennytm added a commit to kennytm/rust that referenced this pull request

May 12, 2018

@kennytm

…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

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request

May 12, 2018

@Mark-Simulacrum

…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

bors added a commit that referenced this pull request

May 12, 2018

@bors

Rollup of 13 pull requests

Successful merges:

Failed merges:

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.