feat(rustdoc-json): Add optional support for rkyv (de)serialization

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2

[LukeMathWalker]

View all comments

Motivation

The JSON documents produced by rustdoc-json are big. More often than not, tools need to access a small fraction of that output—e.g. a couple of types from a transitive dependency, or a subset of the fields on a given rustdoc-json-types type.

Using a binary (de)serialization format and a cache helps to drive down the performance cost of deserialization: you invoke rustdoc-json to get the JSON output you need, re-serialize it using a more perfomant format as target (e.g. bincode or postcard) and thus amortize the cost of future queries that hit the persistent cache rather than rustdoc-json.
This is better, but still not great: the deserialization cost for crates like std still shows up prominently in flamegraphs.

An Alternative Approach: rkyv

rkyv provides a different opportunity: you avoid paying the deserialization cost upfront thanks to zero-copy deserialization.
You're often able to determine if you need a certain entry from the JSON document using the archived version of that type, thus incurring the full deserialization cost only for the subset of items you actually need (example).

The Change

This PR adds support for rkyv behind a feature flag (rkyv_0_8).
For most types, it's a straight-forward derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize) annotation. For co-recursive types, we need to adjust the generated bounds, using the techniques from rkyv's JSON example.

I have added new round-trip tests to ensure rkyv works as expected.

r? @aDotInTheVoid

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

[rustbot]

aDotInTheVoid is currently at their maximum review capacity.
They may take a while to respond.

[aDotInTheVoid]

This makes sense as a thing to add.

View changes since this review

[aDotInTheVoid]

These tests don't run. When I applied

diff --git a/src/rustdoc-json-types/tests.rs b/src/rustdoc-json-types/tests.rs
index e878350e43b..258c22304c3 100644
--- a/src/rustdoc-json-types/tests.rs+++ b/src/rustdoc-json-types/tests.rs@@ -41,6 +41,11 @@ fn test_union_info_roundtrip() {
#[cfg(feature = "rkyv_0_8")]
mod rkyv {
+ #[test]+ fn definenly_fails() {+ panic!("at least the rkyv tests were ran");+ }+
use std::fmt::Debug;
use rkyv::Archive;

Running ./x test ./src/rustdoc-json-types/ still passed.

The fix (I think) is to enable this feature in bootsrap:

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 88f10775333..ab1d2b8a24b 100644
--- a/src/bootstrap/src/core/build_steps/test.rs+++ b/src/bootstrap/src/core/build_steps/test.rs@@ -3302,7 +3302,7 @@ fn run(self, builder: &Builder<'_>) {
builder.kind,
"src/rustdoc-json-types",
SourceType::InTree,
- &[],+ &["rkyv_0_8".to_owned()],
);
// FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy.

(CC @jieyouxu, is this ok to do?)

[LukeMathWalker]

Apologies, I had only tested the crate directly via local cargo test, under the implicit assumption that the testing infrastructure would automatically pick up feature flags for matrix testing.
I've added the feature flag to the bootstrap script, let me know if other changes are needed.

[aDotInTheVoid]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LukeMathWalker]

@rustbot ready

[aDotInTheVoid]

Thanks, I hope this helps perf.

r=me with commits squashed, and when someone from T-bootstrap signs off that 39f7f0d is ok.

View changes since this review

[LukeMathWalker]

Commits have been squashed @aDotInTheVoid. What's the best way to get a reviewer from T-bootstrap (or is your tag in the thread enough)?

[aDotInTheVoid]

I asked on zulip (#t-infra/bootstrap > Review Request: Adding a feature in a test step.). Seems fine. (Sorry, I should've made that clear here).

@bors r+ rollup

[rust-bors]

📌 Commit 1d81c50 has been approved by aDotInTheVoid

It is now in the queue for this repository.

[rust-bors]

📌 Commit 1f05c76 has been approved by aDotInTheVoid

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-bors]

💔 Test for 11041de failed: CI. Failed job:

• try - dist-x86_64-linux-alt (web logs, enhanced plaintext logs)

[JonathanBrouwer]

That's github CI being funny, not this PRs fault...
The fix looks fine to me so lets just keep this approved

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain enhanced)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@rust-timer build 69b11eb

[rust-timer]

Finished benchmarking commit (69b11eb): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -6.0%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-6.0%, -6.0%]	1
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-6.0%	[-6.0%, -6.0%]	1

Cycles

Results (secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.034s -> 479.577s (-0.10%)
Artifact size: 394.90 MiB -> 394.89 MiB (-0.00%)

[aDotInTheVoid]

Published as v0.57.2