Repository navigation
Conversation
|
@reidliu41 Thanks for the patch! Let me take some time to check whether this is the best way forward. When adding the feature in #4869 I was already worrying about the performance implications. |
|
@reidliu41 Technically this looks like a solid approach. Thanks! I'll find some time to give more detailed review comments. Now that we are at it, would you mind doing the same bench again with the release hints turned off? I'm afraid your table above is not giving the full picture. |
Updated the description after applying the suggested changes. |
| let manifest = Manifest::parse(include_str!( | ||
| "../manifest/tests/channel-rust-nightly-example.toml" | ||
| ))?; | ||
| let expected_date = manifest.date.clone(); |
There was a problem hiding this comment.
Nit: It looks like this .clone() is redundant since it is immediately used for equality comparison later? How about just comparing the referenced values?
There was a problem hiding this comment.
Manifest::stringify() currently consumes self, so borrowing
manifest.date across that call would not compile. The test now stringifies
the parsed manifest directly and compares the date read from the serialized
output against the fixture's known date, avoiding the clone.
| let prefix = self.installation.prefix(); | ||
| if let Some(manifest_path) = prefix.dist_manifest() { | ||
| let date = fs::File::open(&manifest_path) | ||
| .map_err(anyhow::Error::from) |
There was a problem hiding this comment.
Nit: This .map_err() looks redundant because it is immediately consumed by debug!() afterwards?
There was a problem hiding this comment.
The conversion is required by and_then: File::open() returns
std::io::Error, while read_manifest_date() returns anyhow::Error.
Removing it fails to compile because and_then requires the same error
type on both sides. Keeping the errors separate would require nested
matches and duplicate the fallback logging, so the current form seems
a little simpler.
42b8a6d to
57206ec
Compare
57206ec to
a0586b0
Compare
Each release-hint check currently reads and parses the whole
multirust-channel-manifest.toml, which is around 33,000 lines and
1 MiB, just to get its date.
Read only the beginning of the manifest and parse the root date. If that
does not work, fall back to the existing full manifest loader.
On my machine:
The numbers below are averages across multiple runs using a 971,571-byte manifest with 32,938 lines:
With release hints enabled, the tested commands are 28–44% faster.
The updated timings are close to the release-hints-disabled baseline.