rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size #98851

klensy · 2022-07-03T16:26:55Z

Encodes strings in rmeta/query cache so duplicated ones will be encoded as offsets to first strings, reducing file size.

cjgillot · 2022-07-03T16:48:20Z

The ICE happens because decoding a Symbol now requires access to the CrateRoot. But decoding the CrateRoot needs to decode Symbols: the very first field is the crate name.

We could build the FxHashMap<u32, Symbol>> up front when creating the MetadataBlob and save it inside it, so we don't need the CrateRoot to decode Symbols.

klensy · 2022-07-03T21:29:31Z

But i can't write symbol table before encoding CrateRoot here

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Lines 2206 to 2210 in 0e21a27

    
           rustc_version().encode(&mut ecx); 
        
           // Encode all the entries and extra information in the crate, 
        
           // culminating in the `CrateRoot` which points to all of it. 
        
           let root = ecx.encode_crate_root();

because some symbols may be missed at that point, and can't reorder table writes, because they immediately written into metadata blob.

cjgillot · 2022-07-03T22:15:11Z

You could put the symbol table after the CrateRoot, and save the in-file position somewhere like we do for CrateRoot.

compiler/rustc_metadata/src/rmeta/decoder.rs

cjgillot · 2022-07-04T17:30:55Z

I just had a (hopefully) simpler idea: use a FxHashMap<Symbol, LazyValue> for encoding. We would encode each string only once, and emit the same LazyValue object for all other occurrences. This does not require to decode a large table upfront.
For decoding, we don't need much special handling. A FxHashMap<LazyValue, Symbol> may allow to reduce traffic to the Symbol interner, but is not required.

klensy · 2022-07-04T18:43:47Z

LazyValue<?> of which type?

cjgillot · 2022-07-04T20:03:37Z

String ?

klensy · 2022-07-05T11:41:01Z

compiler/rustc_metadata/src/rmeta/encoder.rs

+impl<'a, 'tcx> Encodable<EncodeContext<'a, 'tcx>> for Symbol {
+    fn encode(&self, s: &mut EncodeContext<'a, 'tcx>) {
+        if !s.symbol_table.contains_key(self) {
+            let x = s.lazy(self.to_string());


Ok, this looks simpler, but fail on first write here:

thread 'rustc' panicked at 'assertion failed: `(left == right)` left: `NodeStart(30)`, right: `NoNode`', compiler\rustc_metadata\src\rmeta\encoder.rs:397:9

encode_crate_root calls encode_crate_deps that calls self.lazy_array

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Line 1762 in 53792b9

self.lazy_array(deps.iter().map(|&(_, ref dep)| dep))

that change state to LazyState::NodeStart

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Lines 394 to 406 in 53792b9

fn lazy_array<T: ParameterizedOverTcx, I: IntoIterator<Item = B>, B: Borrow<T::Value<'tcx>>>(

&mut self,

values: I,

) -> LazyArray<T>

where

T::Value<'tcx>: Encodable<EncodeContext<'a, 'tcx>>,

{

let pos = NonZeroUsize::new(self.position()).unwrap();

assert_eq!(self.lazy_state, LazyState::NoNode);

self.lazy_state = LazyState::NodeStart(pos);

let len = values.into_iter().map(|value| value.borrow().encode(self)).count();

self.lazy_state = LazyState::NoNode;

Where it founds Symbol and tries to encode it but can't, because of wrong state.

klensy · 2022-07-05T18:05:02Z

Kinda works, size reduced. Perf run please?

Looks like slowdown, that random seeks inside of file is bad.

cjgillot · 2022-07-05T20:08:30Z

@bors try @rust-timer queue

rust-timer · 2022-07-05T20:08:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-05T20:08:46Z

⌛ Trying commit 1ffb3867f739acfafb0d1c6f61aeab0a8f7bf9ca with merge 7d06dad2f206c313c621942c033c8200d8a664b4...

bors · 2022-07-05T21:44:12Z

☀️ Try build successful - checks-actions
Build commit: 7d06dad2f206c313c621942c033c8200d8a664b4 (7d06dad2f206c313c621942c033c8200d8a664b4)

rust-timer · 2022-07-05T21:44:13Z

Queued 7d06dad2f206c313c621942c033c8200d8a664b4 with parent efb171e, future comparison URL.

rust-timer · 2022-07-05T23:30:39Z

Finished benchmarking commit (7d06dad2f206c313c621942c033c8200d8a664b4): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.6%	1.3%	15
Regressions 😿 (secondary)	0.9%	1.3%	22
Improvements 🎉 (primary)	-0.6%	-0.8%	7
Improvements 🎉 (secondary)	-1.5%	-2.3%	10
All 😿🎉 (primary)	0.2%	1.3%	22

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.1%	-3.7%	42
Improvements 🎉 (secondary)	-3.3%	-4.0%	23
All 😿🎉 (primary)	-2.1%	-3.7%	42

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	1.9%	1.9%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.1%	-3.8%	5
All 😿🎉 (primary)	1.9%	1.9%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

klensy · 2022-07-06T10:53:58Z

OK, DecodeContext is bad place to store that table, as it forces reading table from file a lot. Where is a good place to save that table?

A FxHashMap<LazyValue, Symbol> may allow to reduce traffic to the Symbol interner, but is not required.

Where to store that table?

cjgillot · 2022-07-06T11:28:37Z

You can put it in CrateMetadata, which already contains many side tables to accelerate decoding.

klensy · 2022-07-06T12:02:34Z

You can put it in CrateMetadata, which already contains many side tables to accelerate decoding.

And that was implemented in 55032c05a408fdbf61a8113f1a69cef25a86a799 where DecodeContext.cdata wasn't ready at that point, but i'll try again.

cjgillot · 2022-07-06T16:07:11Z

The hash-map would only be there to accelerate decoding, but not necessary for correctness. We could just switch to the slower path of re-decoding + re-interning if cdata is None.

rust-timer · 2022-08-16T17:34:27Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-08-16T17:34:41Z

⌛ Trying commit 7325ba186339602fd7f818f8021262dc7f178e28 with merge 7b8bc869db839305776669feafbbcb22bafffa7f...

bors · 2022-08-16T18:57:52Z

☀️ Try build successful - checks-actions
Build commit: 7b8bc869db839305776669feafbbcb22bafffa7f (7b8bc869db839305776669feafbbcb22bafffa7f)

rust-timer · 2022-08-16T18:57:53Z

Queued 7b8bc869db839305776669feafbbcb22bafffa7f with parent a39bdb1, future comparison URL.

rust-timer · 2022-08-16T20:11:59Z

Finished benchmarking commit (7b8bc869db839305776669feafbbcb22bafffa7f): comparison url.

Instruction count

Primary benchmarks: ❌ relevant regressions found
Secondary benchmarks: ❌ relevant regressions found

	mean¹	max	count²
Regressions ❌ (primary)	0.9%	7.3%	77
Regressions ❌ (secondary)	2.8%	7.4%	53
Improvements ✅ (primary)	-0.9%	-0.9%	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	7.3%	79

Max RSS (memory usage)

Results

Primary benchmarks: ✅ relevant improvements found
Secondary benchmarks: ✅ relevant improvements found

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	3.1%	1
Improvements ✅ (primary)	-1.8%	-2.7%	41
Improvements ✅ (secondary)	-2.5%	-3.2%	25
All ❌✅ (primary)	-1.8%	-2.7%	41

Cycles

Results

Primary benchmarks: ❌ relevant regressions found
Secondary benchmarks: ❌ relevant regressions found

	mean¹	max	count²
Regressions ❌ (primary)	3.5%	7.0%	9
Regressions ❌ (secondary)	4.1%	6.5%	26
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.5%	7.0%	9

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

klensy · 2022-08-16T20:20:04Z

Uhh, pretty bad, need to drop last commit. Remembering, that when i tried different approach of encoding/decoding strings as table, there was similar(?) slowdown, as DecodeContext was created a lot of times. Will try to investigate it later.

klensy · 2022-08-18T10:38:13Z

Let's merge as it is, i dropped last commit that caused regression in #98851 (comment), so it should be like #98851 (comment).

cjgillot · 2022-08-18T20:30:03Z

@bors r+ rollup=never

bors · 2022-08-18T20:30:05Z

📌 Commit adba469 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-08-18T23:53:26Z

⌛ Testing commit adba469 with merge 71ecf5d...

bors · 2022-08-19T02:34:23Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 71ecf5d to master...

rust-timer · 2022-08-19T03:49:15Z

Finished benchmarking commit (71ecf5d): comparison url.

Instruction count

Primary benchmarks: ❌ relevant regressions found
Secondary benchmarks: ❌ relevant regressions found

	mean¹	max	count²
Regressions ❌ (primary)	0.6%	1.5%	15
Regressions ❌ (secondary)	1.1%	1.6%	21
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	-0.6%	2
All ❌✅ (primary)	0.6%	1.5%	15

Max RSS (memory usage)

Results

Primary benchmarks: ✅ relevant improvements found
Secondary benchmarks: ✅ relevant improvements found

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	-4.2%	24
Improvements ✅ (secondary)	-3.2%	-4.5%	25
All ❌✅ (primary)	-2.1%	-4.2%	24

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: ✅ relevant improvements found

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	-4.0%	2
All ❌✅ (primary)	-	-	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-08-19T04:24:30Z

This is a small but consistent slowdown across multiple benchmarks. It also reduces max-rss on a number of benchmarks, mostly for doc builds.

@klensy: what are the file size improvements?

Kobzol · 2022-08-19T05:43:14Z

https://perf.rust-lang.org/compare.html?start=0b79f758c9aa6646606662a6d623a0752286cd17&end=71ecf5d359bf750cc171e124779a46985633439d&stat=size%3Acrate_metadata We can take a look :-) Assuming this is the correct metric.

klensy · 2022-08-19T10:19:31Z

This is a small but consistent slowdown across multiple benchmarks. It also reduces max-rss on a number of benchmarks, mostly for doc builds.

@klensy: what are the file size improvements?

Instructions regression mostly limited to doc build.

Improvements on file sizes:
size:crate_metadata up to 10%
size:linked_artifact up to 8%
size:query_cache up to 4%

This counts also libs with rustc metadata embed (rlibs?), like stdlib/test (can check that on next nightly)

klensy · 2022-08-20T09:20:43Z

Size difference between 0b79f75...e1b28cd of nightly-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib is about -5%, but this affected also by #100209 with strictly bigger metadata size.

pnkfelix · 2022-08-24T14:46:30Z

I suspect the max-rss and file-size improvements are sufficient justification for the regressions witnessed here. (I admit to being on the fence here, but see next bullet ...)
I think the previous bullet is irrelevant anyway, because PR #100803 is in the bors queue and should deal with the fallout here. Yay!

@rustbot label: perf-regression-triaged

…bols, r=bjorn3 Symbols: do not write string values of preinterned symbols into compiled artifacts r? `@bjorn3` Followup for rust-lang#98851 rust-lang#98851 (comment)

rust-highfive assigned cjgillot Jul 3, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2022

This comment has been minimized.

Sign in to view

klensy commented Jul 4, 2022

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs Outdated Show resolved Hide resolved

klensy force-pushed the encode_symbols branch 2 times, most recently from 116ceb7 to 55032c0 Compare July 4, 2022 16:09

klensy commented Jul 5, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 5, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 16, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 16, 2022

klensy force-pushed the encode_symbols branch from 7325ba1 to adba469 Compare August 16, 2022 22:51

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 Aug 18, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2022

bors merged commit 71ecf5d into rust-lang:master Aug 19, 2022

rustbot added this to the 1.65.0 milestone Aug 19, 2022

klensy mentioned this pull request Aug 20, 2022

Symbols: do not write string values of preinterned symbols into compiled artifacts #100803

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Aug 24, 2022

	fn lazy_array<T: ParameterizedOverTcx, I: IntoIterator<Item = B>, B: Borrow<T::Value<'tcx>>>(
	&mut self,
	values: I,
	) -> LazyArray<T>
	where
	T::Value<'tcx>: Encodable<EncodeContext<'a, 'tcx>>,
	{
	let pos = NonZeroUsize::new(self.position()).unwrap();

	assert_eq!(self.lazy_state, LazyState::NoNode);
	self.lazy_state = LazyState::NodeStart(pos);
	let len = values.into_iter().map(\|value\| value.borrow().encode(self)).count();
	self.lazy_state = LazyState::NoNode;

rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size #98851

rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size #98851

Conversation

klensy commented Jul 3, 2022 • edited Loading

cjgillot commented Jul 3, 2022

This comment has been minimized.

klensy commented Jul 3, 2022

cjgillot commented Jul 3, 2022

cjgillot commented Jul 4, 2022

klensy commented Jul 4, 2022

cjgillot commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

klensy commented Jul 5, 2022 • edited Loading

cjgillot commented Jul 5, 2022

rust-timer commented Jul 5, 2022

bors commented Jul 5, 2022

bors commented Jul 5, 2022

rust-timer commented Jul 5, 2022

rust-timer commented Jul 5, 2022

Footnotes

klensy commented Jul 6, 2022

cjgillot commented Jul 6, 2022

klensy commented Jul 6, 2022 • edited Loading

cjgillot commented Jul 6, 2022

rust-timer commented Aug 16, 2022

bors commented Aug 16, 2022

bors commented Aug 16, 2022

rust-timer commented Aug 16, 2022

rust-timer commented Aug 16, 2022

Footnotes

klensy commented Aug 16, 2022 • edited Loading

klensy commented Aug 18, 2022

cjgillot commented Aug 18, 2022

bors commented Aug 18, 2022

bors commented Aug 18, 2022

bors commented Aug 19, 2022

rust-timer commented Aug 19, 2022

Footnotes

nnethercote commented Aug 19, 2022

Kobzol commented Aug 19, 2022

klensy commented Aug 19, 2022 • edited Loading

klensy commented Aug 20, 2022 • edited Loading

pnkfelix commented Aug 24, 2022 • edited Loading

klensy commented Jul 3, 2022 •

edited

Loading

klensy commented Jul 5, 2022 •

edited

Loading

klensy commented Jul 6, 2022 •

edited

Loading

klensy commented Aug 16, 2022 •

edited

Loading

klensy commented Aug 19, 2022 •

edited

Loading

klensy commented Aug 20, 2022 •

edited

Loading

pnkfelix commented Aug 24, 2022 •

edited

Loading