Conversation
rustbot
added
S-waiting-on-review
|
|
nnethercote
force-pushed
the
rewrite-query_ensure_result
branch
from
cf1ff55 to
d7ba9ed
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Let's run perf just in case. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Rewrite `query_ensure_result`.
rustbot
added
the
S-waiting-on-perf
| QueryMode::Ensure { ensure_mode: EnsureMode::Ok }, | ||
| ) { | ||
| // We executed the query. Convert the successful result. | ||
| Some(res) => convert(res), |
There was a problem hiding this comment.
Style: I think value is better than res here (and above), as it’s consistent with the other cache lookups on this file, and we can’t treat the value as a Result without unerasing it first anyway.
|
r=me after considering the |
It currently uses chaining which is concise but hard to read. There are up to four implicit matches occurring after the call to `execute_query_fn`. - The first `map` on `Option<Erased<Result<T, ErrorGuaranteed>>>`. - The second `map` on `Option<Result<T, ErrorGuaranteed>>`. - The third `map` on `Result<T, ErrorGuaranteed>`. - The `unwrap_or` on `Option<Result<(), ErrorGuaranteed>>`. This commit rewrites it to use at most two matches. - An explicit match on `Option<Erased<Result<T, ErrorGuaranteed>>>`. - An explicit match on `Result<T, ErrorGuaranteed>`. This is easier to read. It's also more efficient, though the code isn't hot enough for that to matter.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (936d6e3): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -4.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 481.003s -> 479.608s (-0.29%) |
rustbot
removed
the
S-waiting-on-perf
nnethercote
force-pushed
the
rewrite-query_ensure_result
branch
from
d7ba9ed to
74776c4
Compare
|
@bors r=Zalathar rollup |
rust-bors
bot
added
S-waiting-on-bors
4 participants
It currently uses chaining which is concise but hard to read. There are up to four implicit matches occurring after the call to
execute_query_fn.maponOption<Erased<Result<T, ErrorGuaranteed>>>.maponOption<Result<T, ErrorGuaranteed>>.maponResult<T, ErrorGuaranteed>.unwrap_oronOption<Result<(), ErrorGuaranteed>>.This commit rewrites it to use at most two matches.
Option<Erased<Result<T, ErrorGuaranteed>>>.Result<T, ErrorGuaranteed>.This is easier to read. It's also more efficient, though the code isn't hot enough for that to matter.
r? @Zalathar