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 }})
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.
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.
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)
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
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!
Excellent, thanks for updating!
@bors r+
📌 Commit 6d5ed01 has been approved by michaelwoerister
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
This was referenced
May 10, 2018
bors added a commit that referenced this pull request
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 invec::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:
@michaelwoerister I had to update this for some changes on master after some merges; unsure if that means you have to re-approve?
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)
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).
@michaelwoerister ok, updated (I think)! Let me know if I rebased properly; not entirely sure if I did the right thing :p
📌 Commit 8402a58 has been approved by michaelwoerister
alexcrichton added a commit to alexcrichton/rust that referenced this pull request
…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.
kennytm added a commit to kennytm/rust that referenced this pull request
…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.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request
…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.
bors added a commit that referenced this pull request
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:
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.