-
Notifications
You must be signed in to change notification settings - Fork 13.8k
DepNodeColor
tweaks
#147423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
DepNodeColor
tweaks
#147423
Conversation
Two functions take arguments of this type, but the `Option` is always `Some`, so we can just pass `&MarkFrame<'_>` instead.
Because it's only ever used for inserting red. (`None` is only used as a starting value, and `Green` is inserted by `try_mark_green` and friends.)
Currently it's binary, either `Green` or `Red`. But it's almost always used within an `Option`. So it's a bit neater, and possibly slightly faster, to make it tri-value with `Unknown` as a first-class variant.
@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.
} | ||
} | ||
Red, | ||
Unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Unknown last here, while COMORESSED_UNKNOWN is less than COMPRESSED_RED?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rearranged things so the order is always green, red, unknown.
DepNodeColor::Red, | ||
); | ||
colors.insert_red(SerializedDepNodeIndex::from_u32( | ||
DepNodeIndex::FOREVER_RED_NODE.as_u32(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we introduce a SERIALIZED_RED constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Feels orthogonal to this PR.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (40c8f48): comparison URL. Overall result: ✅ improvements - 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 countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 2.4%)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: 472.01s -> 471.833s (-0.04%) |
Cool, once again the CI results are better than what I saw locally. |
The most common `get` case is green. This commit changes `get` to use use `if`/`else` instead of match, so that getting green requires one comparison instead of two.
It uses a different implementation depending on whether the compiler front-end is running single-threaded or multi-threaded. The two implementations are equivalent and I think the multi-threaded one expresses the intent more clearly, and I imagine the perf is similar. So this commit removes the single-threaded code.
a315aef
to
19423b7
Compare
A follow-up to #147293, where I attempted and mostly failed to make things faster again, but I found a few cleanups worth doing.
r? @ghost