Collector tweaks #67756
Collector tweaks #67756
Conversation
rust-highfive
added
the
S-waiting-on-review
| .into_iter() | ||
| .map(Arc::new) | ||
| .collect::<Vec<_>>() | ||
| let (codegen_units, _) = time(tcx.sess, "partition and assert distinct symbols", || { |
There was a problem hiding this comment.
Why are we only putting these two in parallel? At least it looks like there's no dependency on codegen units until after we collect mono_items, shouldn't that also be a (third) parallel step here?
There was a problem hiding this comment.
That step is pretty cheap so there isn't much point.
There was a problem hiding this comment.
It seems like that's not really for us to decide (and rayon should hopefully deal with "pretty cheap" pretty well), but I'm not opposed to leaving it as is.
| @@ -408,10 +406,12 @@ fn record_accesses<'tcx>( | |||
| mono_item.instantiation_mode(tcx) == InstantiationMode::LocalCopy | |||
| }; | |||
|
|
|||
| let accesses = | |||
| callees.into_iter().map(|mono_item| (*mono_item, is_inlining_candidate(mono_item))); | |||
| let accesses: SmallVec<[_; 128]> = callees | |||
There was a problem hiding this comment.
We should add a comment here about the deadlock between is_inlining_candidate and inlining_map, if I'm following correctly.
There was a problem hiding this comment.
Yes, a deadlock definitely seems noteworthy.
There was a problem hiding this comment.
I'm not sure what to note here other than don't call random code in locks =P
There was a problem hiding this comment.
It is non-obvious to me at least when glancing at the code that there is a lock involved (and that is_inlining_candidate also takes it, if I'm recalling correctly). Explicitly noting that would be good.
There was a problem hiding this comment.
Everything pretty much uses a lock unless you know otherwise. We can't have explicit notes on all function calls.
There was a problem hiding this comment.
My main concern is that someone will drop the SmallVec here as an "optimization". I also feel like the argument about all functions potentially locking is a bit odd - if it were really true, then we'd be unable to review code well. I might even argue that a refactor which explicitly passes the lock in would be helpful here, if possible; it's hopefully rare that we have such unexpected locking.
|
I also think there might be some non-determinism that snuck in here. |
|
☔ The latest upstream changes (presumably #67853) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
collector-tweaks
branch
from
1f6827f
to
4a64716
Compare
|
@bors r=Mark-Simulacrum |
|
📌 Commit 4a64716 has been approved by |
bors
added
S-waiting-on-bors
…acrum Collector tweaks r? @Mark-Simulacrum
…acrum Collector tweaks r? @Mark-Simulacrum
Rollup of 9 pull requests Successful merges: - #67000 (Promote references to constants instead of statics) - #67756 (Collector tweaks) - #67889 (Compile some CGUs in parallel at the start of codegen) - #67930 (Rename Result::as_deref_ok to as_deref) - #68018 (feature_gate: Remove `GateStrength`) - #68070 (clean up E0185 explanation) - #68072 (Fix ICE #68058) - #68114 (Don't require `allow_internal_unstable` unless `staged_api` is enabled.) - #68120 (Ban `...X` pats, harden tests, and improve diagnostics) Failed merges: r? @ghost
…acrum Collector tweaks r? @Mark-Simulacrum
Rollup of 8 pull requests Successful merges: - #67756 (Collector tweaks) - #67889 (Compile some CGUs in parallel at the start of codegen) - #67930 (Rename Result::as_deref_ok to as_deref) - #68018 (feature_gate: Remove `GateStrength`) - #68070 (clean up E0185 explanation) - #68072 (Fix ICE #68058) - #68114 (Don't require `allow_internal_unstable` unless `staged_api` is enabled.) - #68120 (Ban `...X` pats, harden tests, and improve diagnostics) Failed merges: r? @ghost
r? @Mark-Simulacrum