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

rustc_metadata: replace Entry table with one table for each of its fields (AoS -> SoA). #59953

Open
wants to merge 13 commits into
base: master
from

Conversation

@eddyb
Copy link
Member

commented Apr 14, 2019

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)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

⌛️ Trying commit 1ab0e24 with merge 3919374...

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

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

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

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 14, 2019

Success: Queued 3919374 with parent 0085672, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 14, 2019

Finished benchmarking try commit 3919374

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Performance looks good except for webrender-check.

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Performance looks good except for webrender-check.

Can you please be more specific with your performance comments?

Instruction counts for everything look great, including webrender-check. Wall-time for webrender-check check-incremental builds has a 28% regression but almost everything else (including other webrender workloads) is a clear improvement. I wonder if that's a fluky bad measurement. Even if it's not, I think this PR is a clear win overall.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@Zoxc Oh, wow, that's a huge gap between instructions and cycles.
I wonder if it's the extra memory being used, that's causing that.
I really wish I had cache misses on there, it would make things clearer.

Also, I've pushed the extra commit I mentioned in the PR description, let's see what that does!

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

⌛️ Trying commit a173ddc with merge 0403760...

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)
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

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

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 14, 2019

Success: Queued 0403760 with parent 60076bb, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 15, 2019

Finished benchmarking try commit 0403760

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Thanks a lot for the PR, @eddyb! Looks like a nice improvement.
This is a bigger changeset than I thought. I'll review soonish :)

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Keep in mind most commits are refactors that don't change the encoded metadata (or do so without changing its size).
The last two are the most interesting ones wrt this change. Oh and the diff is large in part because indentation/syntax changes.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@Zoxc looks like the latest numbers are better?
(I haven't checked yet between the two builds (for the last 2 commits), and webrender-check may have been spurious, not sure)

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

It's possible, webrender-debug or webrender-opt didn't regress which would be expected if webrender-check did.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Ugh, these numbers are poisoned by the huge delta in 0085672...60076bb.

I'll have to open another PR for the version without the last commit, and start the try builds at the same time...

Centril added a commit to Centril/rust that referenced this pull request May 22, 2019

Rollup merge of rust-lang#61034 - eddyb:soa-metadata-prereq, r=michae…
…lwoerister

rustc_metadata: parametrize schema::CrateRoot by 'tcx and rip out old unused incremental infra.

These are the first two commits of rust-lang#59953, already reviewed and approved by @michaelwoerister.

r? @michaelwoerister

@eddyb eddyb force-pushed the eddyb:soa-metadata branch from 23fd5ad to 8c0ab8c May 24, 2019

@eddyb

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

@Zoxc It's rebased now, and slightly adjusted for #60647 (although I'd like to get rid of PerDefTable by generalizing Table to also take an index type, it doesn't need to be in this PR).

@bors

This comment was marked as resolved.

Copy link
Contributor

commented May 28, 2019

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

@eddyb eddyb force-pushed the eddyb:soa-metadata branch from 8c0ab8c to ff5afe2 May 28, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

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

@@ -1170,57 +1169,57 @@ impl EncodeContext<'_, 'tcx> {
paren_sugar: trait_def.paren_sugar,
has_auto_impl: tcx.trait_is_auto(def_id),
is_marker: trait_def.is_marker,
super_predicates: self.lazy(&tcx.super_predicates_of(def_id)),
super_predicates: self.lazy(&*tcx.super_predicates_of(def_id)),

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

Why does this reborrow?

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 16, 2019

Author Member

&Lrc<T> not being iterable, I think? I'll have to go back and check which of these you switched to &[...] (which would solve the problem, IIRC).

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

GenericPredicates is not iterable either =P

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 16, 2019

Author Member

Oh, sorry, I confused it with an iterable situation. Okay yeah so the problem is I don't want a &Lrc<T> because that will result in Lazy<Lrc<T>> being returned.
But if the Lrc is gone then the &* is redundant.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

The Lrc is gone here.

}
hir::ItemKind::TraitAlias(..) => {
let data = TraitAliasData {
super_predicates: self.lazy(&tcx.super_predicates_of(def_id)),
super_predicates: self.lazy(&*tcx.super_predicates_of(def_id)),

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

Same question here.

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

rustc_metadata: replace LazySeq<T> with Lazy<[T]> looks good to me, you could split that out to another PR and land it.

rustc_metadata: use NonZeroUsize for the position of a Lazy looks like just an optimization. It seems to add a few unwrap calls, so it's no clear if it's a win. Could it be measured separately or after the other commits?

macro_rules! Lazy {
([$T:ty]) => {Lazy<[$T], usize>};
($T:ty) => {Lazy<$T, ()>};
}

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

This macro is pretty ugly. So is just Lazy<T> invariant in T even with the Meta type parameter?

Where do you need the co-variance?

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 16, 2019

Author Member

The default has a projection in it, which variance computation doesn't normalize AFAIK.

I need the variance because e.g. Lazy<Ty<'tcx>> is more like Lazy<for<'tcx> Ty<'tcx>> - you can decode it as any lifetime (that you happen to have a decoder for, that is).
This allows holding CrateRoot<'static> instead of CrateRoot<'tcx> in CrateMetadata.

If we try hard enough, we might be able to make CrateMetadata<'tcx> work, but it might require using a trait object instead of Providers for non-local crates. Or some unsafe code.

@@ -127,15 +128,15 @@ impl<'a, 'tcx> Metadata<'a, 'tcx> for (&'a CrateMetadata, TyCtxt<'a, 'tcx, 'tcx>
}
}

impl<'a, 'tcx: 'a, T: Decodable> Lazy<T> {
impl<'a, 'tcx: 'a, T: Encodable + Decodable> Lazy<T> {

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 16, 2019

Contributor

Where does these encodable bounds come from?

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 16, 2019

Author Member

The whole "lazy metadata" stuff, I was trying to make blanket impls not conflict with "more specialized" impls.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2019

rustc_metadata: use NonZeroUsize for the position of a Lazy looks like just an optimization. It seems to add a few unwrap calls, so it's no clear if it's a win. Could it be measured separately or after the other commits?

It's needed for the changes to Table, so that... Wait, no, I think it was needed while I was still planning to store Vec<Option<Lazy<T>>> but now it's all encoded into bytes, so I can move the transition to NonZeroUsize to the end, and split it into its own PR.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

ping from triage @Zoxc @eddyb any updates on this?

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Jul 26, 2019

Ping from triage @Zocx any updates on this?

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Aug 2, 2019

Ping from triage. @rust-lang/compiler any updates on this?

@JohnTitor

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Ping from triage: @Zoxc @eddyb any updates on this? And there are some merge conflicts to resolve.

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