Only fetch lib_features when there are unknown feature attributes #53444
Conversation
|
@bors try |
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
⌛ Trying commit c4a0235 with merge b0dd0999595bf7fa0484adbda1627f36a88d60f6... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☀️ Test successful - status-travis |
|
@rust-timer build b0dd0999595bf7fa0484adbda1627f36a88d60f6 |
|
Success: Queued b0dd0999595bf7fa0484adbda1627f36a88d60f6 with parent d06fa3a, comparison URL. |
|
I've pushed some more changes which will hopefully improve the situation further. @bors try |
[WIP] Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
|
☀️ Test successful - status-travis |
|
@rust-timer build 2e13a6c |
|
Success: Queued 2e13a6c with parent 8b923a1, comparison URL. |
varkor
changed the title
|
As far as I can tell @nnethercote, this completely negates the performance implications of #52644. Thanks for noticing! This is ready for review. |
src/librustc/middle/lib_features.rs
Outdated
| @@ -143,6 +144,26 @@ impl<'a, 'tcx> Visitor<'tcx> for LibFeatureCollector<'a, 'tcx> { | |||
| NestedVisitorMap::All(&self.tcx.hir) | |||
| } | |||
|
|
|||
| fn visit_item(&mut self, item: &'tcx Item) { | |||
| match item.node { | |||
| ItemKind::ExternCrate(_) => { | |||
There was a problem hiding this comment.
What about Rust 2018, where extern crate is optional? Do inject these statements into HIR? That is, can we rely on them being present even if they come just from Cargo.toml?
There was a problem hiding this comment.
That's a good point; I don't think this will work in the 2018 edition. Let me try tweaking things again.
| // feature attributes if there are non-lang feature attributes, or a crate | ||
| // that depends on the current one has non-lang feature attributes. Thus, | ||
| // we're enabling an arbitrary lib feature to force the check to kick in. | ||
| #![feature(rustc_private)] |
There was a problem hiding this comment.
It seems a bit off that this is necessary? Isn't there a way of making sure these checks always run, if they are needed?
There was a problem hiding this comment.
I should be able to refactor this so we always get the errors.
|
I'm trying a different approach: I'll rebase and comment again if it's successful. Back to WIP. |
varkor
changed the title
This comment has been minimized.
This comment has been minimized.
varkor
force-pushed
the
lib_features-conditional
branch
from
c0aa6c1 to
00ddfc8
Compare
|
@bors try |
[WIP] Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
|
☀️ Test successful - status-travis |
|
@rust-timer build ff2e6f1 |
|
Success: Queued ff2e6f1 with parent bf1e461, comparison URL. |
varkor
force-pushed
the
lib_features-conditional
branch
from
00ddfc8 to
1695856
Compare
varkor
changed the title
|
The perf improvements are slightly worse than the previous iteration, but that's to be expected, because we're iterating through every crate, which is necessary for the 2018 edition. It still minimises the biggest regressions, though and without affecting error messages. @michaelwoerister: ready for another look! (I've rebased because the approach completely changed.) |
|
📌 Commit 1695856 has been approved by |
bors
added
S-waiting-on-bors
…ister Only fetch lib_features when there are unknown feature attributes An attempt to win back some of the performance lost in #52644 (comment). cc @nnethercote
|
☀️ Test successful - status-appveyor, status-travis |
An attempt to win back some of the performance lost in #52644 (comment).
cc @nnethercote