Liveness analysis for everybody #77281
Conversation
|
r? @oli-obk (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
tmiasko
force-pushed
the
liveness-everybody
branch
from
ffe0c44
to
e39c078
Compare
|
@bors try @rust-timer queue I don't care whether this has a perf impact, but I'd like to know before merging so we make a concious decision about it |
|
Awaiting bors try build completion |
|
⌛ Trying commit e39c078321402714fafe8907a275515637bcfb9d with merge 775f8455dd97e8fb2f22eeb9a943bece2c4a64f4... |
| if let FnKind::Method(..) = fk { | ||
| let parent = self.tcx.hir().get_parent_item(id); | ||
| if let Node::ImplItem(..) = self.tcx.hir().get(hir_id) { | ||
| let parent = self.tcx.hir().get_parent_item(hir_id); | ||
| if let Some(Node::Item(i)) = self.tcx.hir().find(parent) { | ||
| if i.attrs.iter().any(|a| self.tcx.sess.check_name(a, sym::automatically_derived)) { | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if the parent relation in the DefId space is set up like this, but can you try whether
if let Some(parent) = self.tcx.parent(def_id) {
if self.tcx.has_attr(parent, sym::automatically_derived) {
return;
}
}also works? If so, please use that
There was a problem hiding this comment.
Yep, that works as well.
Part of the motivation for the current form of the check was to avoid using the automatically_derived attribute if it were to occur in completely unexpected place, like a crate or a module. Should I change it anyway?
There was a problem hiding this comment.
I think we should prevent such uses via other means, but you can also add an assert_eq!(self.tcx.def_kind(parent), DefKind::Impl) before the return to make sure.
There was a problem hiding this comment.
We do not prevent such uses this right now, so I used this as a part of condition. I would like to be extra conservative about any additional uses of this attribute since I think it is stabilized unintentionally and its purpose has changed over time.
There was a problem hiding this comment.
oh :/ that's not great. Yea let's go with a non-panicking version for now
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 775f8455dd97e8fb2f22eeb9a943bece2c4a64f4 with parent 535d27a, future comparison URL. |
|
Finished benchmarking try commit (775f8455dd97e8fb2f22eeb9a943bece2c4a64f4): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Perform liveness analysis for every body instead of limiting it to fns.
tmiasko
force-pushed
the
liveness-everybody
branch
from
e39c078
to
924e8aa
Compare
|
The performance impact is limited, except for the many-assoc-items benchmark which contains mostly associated consts. To a large degree we were paying the price of the computation already. The previous code was creating variables and lives nodes for all bodies, it just didn't always run the liveness analysis. Furthermore, when the liveness analysis didn't run, the live nodes and variables were included in the analysis of a parent if any. |
|
@bors r+ I was expecting some sort of perf hit on assoc const heavy code, but this is a bugfix, so let's roll with it |
|
📌 Commit 924e8aa has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
Perform liveness analysis for every body instead of limiting it to fns.
Fixes #77169.