rustc_metadata: Support non-`Option` nullable values in metadata tables #107166

petrochenkov · 2023-01-21T18:28:09Z

This is a convenience feature for cases in which "no value in the table" and "default value in the table" are equivalent.
Tables using Table<DefIndex, ()> are migrated in this PR, some other cases can be migrated later.
This helps DocFlags in #107136 in particular.

rustbot · 2023-01-21T18:28:14Z

r? @oli-obk

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

petrochenkov · 2023-01-21T18:29:25Z

This shouldn't affect performance if inlining is decent, but let's check.
@bors try @rust-timer queue

bors · 2023-01-21T18:29:34Z

⌛ Trying commit cb57ec1aea24c81c20fd61e2aac5e2840d34bf77 with merge 377b124a5ae81e42de3f95818dea84ff400107f1...

bors · 2023-01-21T20:35:35Z

☀️ Try build successful - checks-actions
Build commit: 377b124a5ae81e42de3f95818dea84ff400107f1 (377b124a5ae81e42de3f95818dea84ff400107f1)

rust-timer · 2023-01-22T00:39:18Z

Finished benchmarking commit (377b124a5ae81e42de3f95818dea84ff400107f1): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	2
Improvements ✅ (secondary)	-0.7%	[-0.8%, -0.3%]	7
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-4.9%, -2.5%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.1%	[5.1%, 6.9%]	3
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.1%	[5.1%, 6.9%]	3

petrochenkov · 2023-01-22T08:06:20Z

@bors rollup=maybe

cjgillot · 2023-01-22T09:16:07Z

compiler/rustc_metadata/src/rmeta/mod.rs

+    is_intrinsic: Table<DefIndex, bool>,
+    is_macro_rules: Table<DefIndex, bool>,
+    is_type_alias_impl_trait: Table<DefIndex, bool>,
+    may_have_doc_links: Table<DefIndex, bool>,


Can explicit_item_bounds and inferred_outlives_of be migrated to this setup?

Looks like it's a bit more involved because you need to make the table.get(...).decode(...).process_decoded(...) chain fast for LazyArrays without using None to skip all the extra work.
I'll try, but probably closer to the next weekend.

r=me on this PR if you want to do that in a separate PR

oli-obk · 2023-01-23T08:43:18Z

compiler/rustc_metadata/src/rmeta/table.rs

+}
+
+impl<I: Idx, const N: usize, T: FixedSizeEncoding<ByteArray = [u8; N]>> TableBuilder<I, T> {
+    pub(crate) fn set_nullable(&mut self, i: I, value: T) {


I think we should have a convenience wrapper around set_nullable for TableBuilder<I, bool> to just set it to true. Invoking set_nullable(val, false) is never done and likely a bug.

Together with #107166 (comment) I'll try to move the true/false conditions to inside the set methods.
So it will look approximately like this

fn set(value) { if value.is_default() { return; } // possibly resize the buffer and set the non-default value }

and false/None/etc arguments will be accepted but result in doing nothing.

The "reset to None" semantic is never currently used for metadata tables, but if it will be needed it will require a separate method like fn clear in this case.

petrochenkov · 2023-01-23T10:14:32Z

@bors r=oli-obk

bors · 2023-01-23T10:14:34Z

📌 Commit cb57ec1aea24c81c20fd61e2aac5e2840d34bf77 has been approved by oli-obk

It is now in the queue for this repository.

This is a convenience feature for cases in which "no value in the table" and "default value in the table" are equivalent. Tables using `Table<DefIndex, ()>` are migrated in this PR, some other cases can be migrated later. This helps `DocFlags` in rust-lang#107136 in particular.

petrochenkov · 2023-01-23T16:32:48Z

@bors r=oli-obk

bors · 2023-01-23T16:32:50Z

📌 Commit 91fdbd7 has been approved by oli-obk

It is now in the queue for this repository.

rustc_metadata: Support non-`Option` nullable values in metadata tables This is a convenience feature for cases in which "no value in the table" and "default value in the table" are equivalent. Tables using `Table<DefIndex, ()>` are migrated in this PR, some other cases can be migrated later. This helps `DocFlags` in rust-lang#107136 in particular.

Rollup of 9 pull requests Successful merges: - rust-lang#105552 (Add help message about function pointers) - rust-lang#106583 (Suggest coercion of `Result` using `?`) - rust-lang#106767 (Allow setting CFG_DISABLE_UNSTABLE_FEATURES to 0) - rust-lang#106823 (Allow fmt::Arguments::as_str() to return more Some(_).) - rust-lang#107166 (rustc_metadata: Support non-`Option` nullable values in metadata tables) - rust-lang#107213 (Add suggestion to remove if in let..else block) - rust-lang#107223 (`sub_ptr()` is equivalent to `usize::try_from().unwrap_unchecked()`, not `usize::from().unwrap_unchecked()`) - rust-lang#107227 (`new_outside_solver` -> `evaluate_root_goal`) - rust-lang#107232 (rustdoc: simplify settings popover DOM, CSS, JS) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustc_metadata: Encode/decode some `LazyArrays` without an `Option` and a couple of related changes, see individual commits. Addresses comments in rust-lang#107166 (comment) and rust-lang#107166 (comment), cc `@cjgillot` `@oli-obk.`

rustbot assigned oli-obk Jan 21, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 21, 2023

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 Jan 21, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2023

cjgillot reviewed Jan 22, 2023

View reviewed changes

oli-obk reviewed Jan 23, 2023

View reviewed changes

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 Jan 23, 2023

This comment was marked as resolved.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 23, 2023

petrochenkov force-pushed the nooptable branch from cb57ec1 to 91fdbd7 Compare January 23, 2023 16:32

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2023

Dylan-DPC mentioned this pull request Jan 24, 2023

Rollup of 7 pull requests #107249

Closed

Dylan-DPC mentioned this pull request Jan 25, 2023

Rollup of 9 pull requests #107290

Merged

bors merged commit a788ce2 into rust-lang:master Jan 25, 2023

rustbot added this to the 1.69.0 milestone Jan 25, 2023

petrochenkov mentioned this pull request Feb 5, 2023

rustc_metadata: Encode/decode some LazyArrays without an Option #107693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_metadata: Support non-`Option` nullable values in metadata tables #107166

rustc_metadata: Support non-`Option` nullable values in metadata tables #107166

petrochenkov commented Jan 21, 2023

rustbot commented Jan 21, 2023

petrochenkov commented Jan 21, 2023

This comment has been minimized.

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

rust-timer commented Jan 22, 2023

petrochenkov commented Jan 22, 2023

cjgillot Jan 22, 2023

petrochenkov Jan 22, 2023 •

edited

Loading

oli-obk Jan 23, 2023

oli-obk Jan 23, 2023

petrochenkov Jan 23, 2023

petrochenkov commented Jan 23, 2023

bors commented Jan 23, 2023

This comment was marked as resolved.

petrochenkov commented Jan 23, 2023

bors commented Jan 23, 2023

rustc_metadata: Support non-Option nullable values in metadata tables #107166

rustc_metadata: Support non-Option nullable values in metadata tables #107166

Conversation

petrochenkov commented Jan 21, 2023

rustbot commented Jan 21, 2023

petrochenkov commented Jan 21, 2023

This comment has been minimized.

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

rust-timer commented Jan 22, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

petrochenkov commented Jan 22, 2023

cjgillot Jan 22, 2023

Choose a reason for hiding this comment

petrochenkov Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk Jan 23, 2023

Choose a reason for hiding this comment

oli-obk Jan 23, 2023

Choose a reason for hiding this comment

petrochenkov Jan 23, 2023

Choose a reason for hiding this comment

petrochenkov commented Jan 23, 2023

bors commented Jan 23, 2023

This comment was marked as resolved.

petrochenkov commented Jan 23, 2023

bors commented Jan 23, 2023

rustc_metadata: Support non-`Option` nullable values in metadata tables #107166

rustc_metadata: Support non-`Option` nullable values in metadata tables #107166

petrochenkov Jan 22, 2023 •

edited

Loading