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

Revert two unapproved changes to rustc_typeck. #59789

Open
wants to merge 5 commits into
base: master
from

Conversation

@eddyb
Copy link
Member

commented Apr 8, 2019

There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to rustc_typeck's "collect" queries, for rustdoc, that were neither needed nor correct.
I'm reverting them here, and will fix up rustdoc somehow, if necessary.

cc @rust-lang/compiler How do we ensure this doesn't happen again?

r? @nikomatsakis or @oli-obk

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

cc @rust-lang/compiler How do we ensure this doesn't happen again?

We could setup bors to require a github approval from a relevant team member on certain source paths.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

How do we ensure this doesn't happen again?

Sigh, resolve has a bunch of rustdoc-specific (and diagnostics-specific) hacks that don't work entirely correctly, but at least they don't affect the normal compiler run.
I ended up letting them live, not blocking PRs introducing them, and just hoping to remove or refactor them into something more correct and aesthetically pleasing some day.
If I had all the time in the world, I'd of course took every PR that I think could be done better and rewrote it properly, but a man can dream...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

If I had all the time in the world, I'd of course took every PR from GuillaumeGomez and rewrote it properly, but a man can dream...

All of them have been approved except for the two last ones (and on the last one I did ping the compiler team!). If not happy with the PR, you can comment on it.

By merging this PR, you'll reintroduce bugs in rustdoc. I'm against this changes. The process hasn't been followed, I'm sorry about that, but instead, wouldn't it be better to fix the librustc_typeck bad changes I made directly? As far as I can tell, it doesn't break anything in the rustc internals whereas reverting will break rustdoc again.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:03becf5e:start=1554707282779199429,finish=1554707373412668843,duration=90633469414
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:59:11] 
[00:59:11] error: Could not document `alloc`.
[00:59:11] 
[00:59:11] Caused by:
[00:59:11]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2018 --crate-name alloc src/liballoc/lib.rs --color always --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/doc --markdown-css rust.css --markdown-no-toc --generate-redirect-pages --resource-suffix 1.35.0 --index-page /checkout/src/doc/index.md -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcompiler_builtins-983bfd746ea1187e.rmeta --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-c94a89edb07c8a5f.rmeta` (exit code: 1)
[00:59:11] 
[00:59:11] 
[00:59:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-Z" "unstable-options" "-p" "alloc" "--" "--markdown-css" "rust.css" "--markdown-no-toc" "--generate-redirect-pages" "--resource-suffix" "1.35.0" "--index-page" "/checkout/src/doc/index.md"
[00:59:11] 
[00:59:11] 
[00:59:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap doc
[00:59:11] Build completed unsuccessfully in 0:07:17
---
travis_time:end:1c44423a:start=1554710935638239108,finish=1554710935648839095,duration=10599987
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:123c2b2f
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:10a69316
$ dmesg | grep -i kill

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)

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I opened #59790 for further discussion. This PR should of course not break rustdoc, but address the original issues in a manner not affecting regular compilation.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

If not happy with the PR, you can comment on it.

There should be an explicit review and approval, i.e. default to not merge non-rustc changes to rustc crates, instead of defaulting to merge in the absence of requested changes.

As far as I can tell, it doesn't break anything in the rustc internals whereas reverting will break rustdoc again.

As I've explained in #59790 (comment), this is undocumented cruft, and as such it will cause confusion or even stop some refactors, if nobody checks the change history and just assumes the code is needed.

And I obviously can't break rustdoc, since we have tests! So I'm forced to do a minimal fix, at least.


@petrochenkov FWIW, I think rustdoc as a whole needs some refactors, and @rust-lang/rustdoc has to make due with the codebase they've inherited. I've offered a few times but ended up with not enough spare time to do said refactors (although maybe I should try again, e.g. the comments I left on #58894).

Overall I'd rather stick the blame on the grandfathered codebase, and process breakdown, than individuals.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

If #59004 and #58894 were the PRs that introduced the problematic changes, we can revert them wholesale. They were an enhancement and cosmetic fix, respectively, and reverting them should not introduce any show-stopping bugs to rustdoc.

It wouldn't hurt to get more eyes on rustdoc PRs more generally; i know i've been struggling to stay on top of my review queue, and having more hands would help keep everything going smoothly.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@QuietMisdreavus no need, for #59004 I already have an equivalent replacement (since the data is already in rustdoc, no need to ask typeck to recompute it), while for #58894 I almost already have the proper solution implemented (cross-crate explicit_predicates_of, which is mostly copy-paste).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@QuietMisdreavus Considering that #59004 is a big search improvement, it'd be quite the regression to revert. Luckily for me, it's not needed. 😌

@eddyb eddyb force-pushed the eddyb:typeck-reverts branch from 94647bb to 76166f6 Apr 9, 2019

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@bors try (let's see if I made anything significantly slower)

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

⌛️ Trying commit 76166f6 with merge b2e5868...

bors added a commit that referenced this pull request Apr 9, 2019

Auto merge of #59789 - eddyb:typeck-reverts, r=<try>
Revert two unapproved changes to rustc_typeck.

There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct.
I'm reverting them here, and will fix up rustdoc *somehow*, if necessary.

cc @rust-lang/compiler How do we ensure this doesn't happen again?

r? @nikomatsakis or @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

☀️ Try build successful - checks-travis
Build commit: b2e5868

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 10, 2019

Success: Queued b2e5868 with parent 3750348, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 10, 2019

Finished benchmarking try commit b2e5868

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

cc @nikomatsakis @michaelwoerister @nnethercote How bad are the perf results?

I could try to supply predicates_defined_on (which is equivalent to predicates_of except for traits) from rustc_metadata by deserializing the explicit_predicates_of and then deserializing and appending the inferred_outlives_of without allocating either into an Lrc, but I'm not sure that's needed.

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

The worst-affected benchmarks are the short-running, less important ones. But it's still clearly a regression :(

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Here is a Cachegrind diff for a "Check CleanIncr" build of ripgrep, which was one the biggest regressions in a larger benchmark.

--------------------------------------------------------------------------------
Ir                 
--------------------------------------------------------------------------------
9,063,571 (100.0%)  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir                   file:function
--------------------------------------------------------------------------------
-1,355,130 (-15.0%)  /home/njn/moz/rustN/src/libserialize/serialize.rs:serialize::serialize::Decoder::read_struct_field
   899,266 ( 9.92%)  /home/njn/moz/rustN/src/librustc_metadata/decoder.rs:rustc_metadata::decoder::DecodeContext::read_lazy_distance
   772,315 ( 8.52%)  /home/njn/moz/rustN/src/libserialize/serialize.rs:<rustc_metadata::schema::Entry as serialize::serialize::Decodable>::decode
   647,102 ( 7.14%)  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::dep_graph::prev::PreviousDepGraph::new
   575,554 ( 6.35%)  /home/njn/moz/rustN/src/libcore/ptr.rs:rustc::dep_graph::prev::PreviousDepGraph::new
   493,118 ( 5.44%)  /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:rustc::dep_graph::prev::PreviousDepGraph::new
   431,526 ( 4.76%)  /home/njn/moz/rustN/src/librustc_metadata/decoder.rs:<rustc_metadata::schema::Entry as serialize::serialize::Decodable>::decode
  -408,336 (-4.51%)  /home/njn/moz/rustN/src/librustc_metadata/schema.rs:<rustc_metadata::schema::Entry as serialize::serialize::Decodable>::decode
   380,927 ( 4.20%)  /home/njn/moz/rustN/src/libcore/intrinsics.rs:rustc::dep_graph::prev::PreviousDepGraph::new
   379,621 ( 4.19%)  /home/njn/moz/rustN/src/libserialize/opaque.rs:<serialize::opaque::Decoder as serialize::serialize::Decoder>::read_usize
   338,776 ( 3.74%)  /home/njn/moz/rustN/src/libserialize/leb128.rs:rustc_incremental::persist::load::load_dep_graph::{{closure}}::{{closure}}
   301,719 ( 3.33%)  /home/njn/moz/rustN/src/librustc/dep_graph/graph.rs:rustc::dep_graph::graph::DepGraph::try_mark_previous_green
   293,040 ( 3.23%)  /home/njn/moz/rustN/src/libserialize/serialize.rs:<u32 as serialize::serialize::Encodable>::encode
   281,777 ( 3.11%)  /home/njn/moz/rustN/src/libserialize/leb128.rs:<u32 as serialize::serialize::Encodable>::encode
   276,688 ( 3.05%)  /home/njn/moz/rustN/src/libserialize/leb128.rs:<serialize::opaque::Decoder as serialize::serialize::Decoder>::read_usize
   245,283 ( 2.71%)  /home/njn/moz/rustN/src/libcore/num/mod.rs:rustc::dep_graph::prev::PreviousDepGraph::new
   217,059 ( 2.39%)  /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:rustc::dep_graph::graph::CurrentDepGraph::intern_node
  -189,246 (-2.09%)  /home/njn/moz/rustN/src/librustc_metadata/decoder.rs:serialize::serialize::Decoder::read_struct_field
   183,414 ( 2.02%)  /home/njn/moz/rustN/src/librustc/dep_graph/graph.rs:rustc::dep_graph::graph::CurrentDepGraph::intern_node
   177,540 ( 1.96%)  /home/njn/.cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-0.6.7/lib.rs:smallvec::SmallVec<A>::push
   169,388 ( 1.87%)  /home/njn/moz/rustN/src/liballoc/vec.rs:<u32 as serialize::serialize::Encodable>::encode
   151,618 ( 1.67%)  /home/njn/moz/rustN/src/libcore/slice/mod.rs:rustc::dep_graph::graph::DepGraph::try_mark_previous_green
   148,582 ( 1.64%)  /home/njn/moz/rustN/src/libcore/slice/mod.rs:rustc::dep_graph::graph::DepGraph::serialize
   129,970 ( 1.43%)  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc_resolve::Resolver::set_binding_parent_module
   129,382 ( 1.43%)  /home/njn/moz/rustN/<::rustc::ty::codec::__impl_decoder_methods macros>:<rustc_metadata::schema::Entry as serialize::serialize::Decodable>::decode
   122,276 ( 1.35%)  /home/njn/moz/rustN/src/librustc_data_structures/fingerprint.rs:rustc_data_structures::fingerprint::Fingerprint::encode_opaque
   103,533 ( 1.14%)  /home/njn/moz/rustN/src/libcore/slice/mod.rs:<serialize::opaque::Decoder as serialize::serialize::Decoder>::read_usize
   100,119 ( 1.10%)  /home/njn/moz/rustN/src/librustc/ty/query/on_disk_cache.rs:rustc::ty::query::on_disk_cache::OnDiskCache::load_diagnostics
    99,423 ( 1.10%)  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::dep_graph::graph::CurrentDepGraph::intern_node
    93,258 ( 1.03%)  /home/njn/moz/rustN/src/libserialize/serialize.rs:rustc_incremental::persist::load::load_dep_graph::{{closure}}::{{closure}}

@eddyb eddyb force-pushed the eddyb:typeck-reverts branch from 76166f6 to 6cd0bef Apr 10, 2019

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

⌛️ Trying commit 6cd0bef with merge 7310273...

bors added a commit that referenced this pull request Apr 10, 2019

Auto merge of #59789 - eddyb:typeck-reverts, r=<try>
Revert two unapproved changes to rustc_typeck.

There was a breakdown in process (#59004 (comment), #58894 (comment)) and two changes were made to `rustc_typeck`'s "collect" queries, for rustdoc, that were neither needed *nor* correct.
I'm reverting them here, and will fix up rustdoc *somehow*, if necessary.

cc @rust-lang/compiler How do we ensure this doesn't happen again?

r? @nikomatsakis or @oli-obk
@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@nnethercote Now the encoding/decoding should be the same as before in most cases.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

☀️ Try build successful - checks-travis
Build commit: 7310273

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

The "Check CleanIncr" results for ripgrep were barely changed by the updated code.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Ugh, I completely misunderstood the slowdown.
It's caused by going intorustc_metadata at all. Every cross-crate query deserializes the Entry for a DefId over and over again, and that's not cheap...
I guess I'll look into ways of mitigating that, which should make this patch negligible.

bors added a commit that referenced this pull request Apr 14, 2019

Auto merge of #59953 - eddyb:soa-metadata, r=<try>
 rustc_metadata: replace Entry table with one table for each of its fields (AoS -> SoA).

In #59789 (comment) I noticed that for many cross-crate queries (e.g. `predicates_of(def_id)`), we were deserializing the `rustc_metadata::schema::Entry` for `def_id` *only* to read one field (i.e. `predicates`).

But there are several such queries, and `Entry` is not particularly small (in terms of number of fields, the encoding itself is quite compact), so there is a large (and unnecessary) constant factor.

This PR replaces the (random-access) array¹ of `Entry` structures ("AoS"), with many separate arrays¹, one for each field that used to be in `Entry` ("SoA"), resulting in the ability to read individual fields separately, with negligible time overhead (in thoery), and some size overhead (as these arrays are not sparse).

For stage1 `libcore`'s metadata blob, the size overhead is `8.44%`, and I have another commit (not initially included in this PR because I want to do perf runs with both) that brings it down to `5.88%`.

¹(in the source, these arrays are called "tables", but perhaps they could use a better name)

bors added a commit that referenced this pull request Apr 14, 2019

Auto merge of #59953 - eddyb:soa-metadata, r=<try>
 rustc_metadata: replace Entry table with one table for each of its fields (AoS -> SoA).

*Based on top of #59887*

In #59789 (comment) I noticed that for many cross-crate queries (e.g. `predicates_of(def_id)`), we were deserializing the `rustc_metadata::schema::Entry` for `def_id` *only* to read one field (i.e. `predicates`).

But there are several such queries, and `Entry` is not particularly small (in terms of number of fields, the encoding itself is quite compact), so there is a large (and unnecessary) constant factor.

This PR replaces the (random-access) array¹ of `Entry` structures ("AoS"), with many separate arrays¹, one for each field that used to be in `Entry` ("SoA"), resulting in the ability to read individual fields separately, with negligible time overhead (in thoery), and some size overhead (as these arrays are not sparse).

In a way, the new approach is closer to incremental on-disk caches, which store each query's cached results separately, but it would take significantly more work to unify the two.

For stage1 `libcore`'s metadata blob, the size overhead is `8.44%`, and I have another commit (not initially included in this PR because I want to do perf runs with both) that brings it down to `5.88%`.

¹(in the source, these arrays are called "tables", but perhaps they could use a better name)
@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

I've opened #59953 with the tentative performance fix, which should reduce the overhead of this PR.

bors added a commit that referenced this pull request Apr 17, 2019

Auto merge of #59953 - eddyb:soa-metadata, r=<try>
 rustc_metadata: replace Entry table with one table for each of its fields (AoS -> SoA).

*Based on top of #59887*

In #59789 (comment) I noticed that for many cross-crate queries (e.g. `predicates_of(def_id)`), we were deserializing the `rustc_metadata::schema::Entry` for `def_id` *only* to read one field (i.e. `predicates`).

But there are several such queries, and `Entry` is not particularly small (in terms of number of fields, the encoding itself is quite compact), so there is a large (and unnecessary) constant factor.

This PR replaces the (random-access) array¹ of `Entry` structures ("AoS"), with many separate arrays¹, one for each field that used to be in `Entry` ("SoA"), resulting in the ability to read individual fields separately, with negligible time overhead (in thoery), and some size overhead (as these arrays are not sparse).

In a way, the new approach is closer to incremental on-disk caches, which store each query's cached results separately, but it would take significantly more work to unify the two.

For stage1 `libcore`'s metadata blob, the size overhead is `8.44%`, and I have another commit (~~not initially included because I want to do perf runs with both~~ **EDIT**: added it now) that brings it down to `5.88%`.

¹(in the source, these arrays are called "tables", but perhaps they could use a better name)

bors added a commit that referenced this pull request Apr 17, 2019

Auto merge of #59953 - eddyb:soa-metadata, r=<try>
 rustc_metadata: replace Entry table with one table for each of its fields (AoS -> SoA).

*Based on top of #59887*

In #59789 (comment) I noticed that for many cross-crate queries (e.g. `predicates_of(def_id)`), we were deserializing the `rustc_metadata::schema::Entry` for `def_id` *only* to read one field (i.e. `predicates`).

But there are several such queries, and `Entry` is not particularly small (in terms of number of fields, the encoding itself is quite compact), so there is a large (and unnecessary) constant factor.

This PR replaces the (random-access) array¹ of `Entry` structures ("AoS"), with many separate arrays¹, one for each field that used to be in `Entry` ("SoA"), resulting in the ability to read individual fields separately, with negligible time overhead (in thoery), and some size overhead (as these arrays are not sparse).

In a way, the new approach is closer to incremental on-disk caches, which store each query's cached results separately, but it would take significantly more work to unify the two.

For stage1 `libcore`'s metadata blob, the size overhead is `8.44%`, and I have another commit (~~not initially included because I want to do perf runs with both~~ **EDIT**: added it now) that brings it down to `5.88%`.

¹(in the source, these arrays are called "tables", but perhaps they could use a better name)
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@eddyb so is this PR blocked on #59953? what's the story here?

@eddyb

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@nikomatsakis Yeah, and that's waiting for a review from @michaelwoerister.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.