Remove MonoItems.
#112126
Remove MonoItems.
#112126
Conversation
PR rust-lang#97168 was created to address this comment in `record_accesses`: // We collect this into a `SmallVec` to avoid calling `is_inlining_candidate` in the lock. // FIXME: Call `is_inlining_candidate` when pushing to `neighbors` in `collect_items_rec` // instead to avoid creating this `SmallVec`. It removed the need for the `SmallVec` (a good thing) by introducing `MonoItems`, which introduced some additional complexity (a bad thing). However, the reason the `SmallVec` was being used in the first place -- "to avoid calling `is_inlining_candidate` in the lock" -- isn't necessary, as far as I can tell. Perhaps it was at one point. So this commit just moves what is left of the call to `is_inlining_candidate` -- a call the innocuous `instantiation_mode` -- inside the lock. This avoids the complexity of `MonoItems` sometimes computing inlining data and sometimes not.
rustbot
added
S-waiting-on-review
|
cc @SparrowLii, @cjgillot |
| self.targets.push(*mono_item); | ||
| if *inlined { | ||
| let is_inlined = mono_item.instantiation_mode(tcx) == InstantiationMode::LocalCopy; |
There was a problem hiding this comment.
The key lies in the cost of calling instantiation_mode. Calling it within the lock will increase the locking time. It's not clear to me that if this has a noticeable effect on the efficiency of monomorphization. cc @Zoxc
There was a problem hiding this comment.
The comment was added in #67756. Reviewers asked for more explanation in comments, but the PR author refused. It seems like it was for a deadlock. I ran the ui tests with the parallel front-end enabled without problem.
There was a problem hiding this comment.
#112128 is an alternative approach that avoids the whole locking issue.
There was a problem hiding this comment.
instantiation_mode uses queries so it could potentially invoke a lot of the compiler (including the query deadlock handler). I'm not confident this is unproblematic here (ignoring the performance reasons to not do it). I'd rather keep code inside locks small and predictable.
|
#112128 is a better approach. |
PR #97168 was created to address this comment in
record_accesses:It removed the need for the
SmallVec(a good thing) by introducingMonoItems, which introduced some additional complexity (a bad thing).However, the reason the
SmallVecwas being used in the first place -- "to avoid callingis_inlining_candidatein the lock" -- isn't necessary, as far as I can tell. Perhaps it was at one point.So this commit just moves what is left of the call to
is_inlining_candidate-- a call the innocuousinstantiation_mode-- inside the lock. This avoids the complexity ofMonoItemssometimes computing inlining data and sometimes not.r? @wesleywiser