Stream: t-compiler/wg-incr-comp

Topic: Status report on #79519


view this post on Zulip cjgillot (Feb 10 2021 at 17:50):

@wg-incr-comp: I was watching the last meeting's video, and a status update on #79519 is useful.

The objective of the PR is to remove all the &[Attribute] slices from the HIR.
The operational assumption is that attributes only impact one pass of the compilation process (eg: allow one lint). Therefore, removing them from the HIR would allow to reuse a lot of analyses.
Technically, the attributes are stored out of the HIR tree in a HirId -> &[Attribute] map. This map is accessible through a hir_attrs(HIR owner) query. The commits modify the HIR nodes one by one along with the fallout in other crates. A few commits contain refactorings, btu not nearly enough.
The first version used nested IndexVecs, and had a ~5% instruction-count regression.
The latest version uses a BTreeMap<HirId, &[Attribute]>, considering attributes are rare and this map should be sparse. The instruction-count regression is at ~2%.

I am out of ideas on how to remove this perf regression.

view this post on Zulip pnkfelix (Feb 10 2021 at 17:55):

is the ThinIndexVec idea unlikely to have a better performance profile than the BTreeMap ?

view this post on Zulip Wesley Wiser (Feb 10 2021 at 18:11):

pnkfelix said:

is the ThinIndexVec idea unlikely to have a better performance profile than the BTreeMap ?

To expand on this, the idea was what I mentioned here and to try using a IndexVec<LocalDefId, ThinIndexVec<ItemLocalId, &'tcx [Attribute]>> instead of the original IndexVec<LocalDefId, IndexVec<ItemLocalId, &'tcx [Attribute]>> which is now a BTreeMap. We could then compare the BTreeMap performance vs the IndexVec<LocalDefId, ThinIndexVec<ItemLocalId, &'tcx [Attribute]>> performance and see which is better.

view this post on Zulip cjgillot (Feb 10 2021 at 18:42):

I haven't tried ThinIndexVec yet (missed that part of your comment).
I am not sure it would help: going from IndexVec to ThinIndex is a pessimisation in terms of instruction count.
BTreeMap is already less-bad in instr-count, and a win in max-rss.
Nevertheless, I will try the ThinIndexVec just to be sure.

view this post on Zulip Wesley Wiser (Feb 10 2021 at 19:43):

No worries! The BTreeMap helped a lot more than I thought it would but it would still be interesting to compare with the ThinIndexVec idea.

view this post on Zulip cjgillot (Feb 10 2021 at 20:08):

-> #81970

view this post on Zulip cjgillot (Feb 10 2021 at 22:40):

Perf results for ThinIndexVec in https://perf.rust-lang.org/compare.html?start=07194ffcd25b0871ce560b9f702e52db27ac9f77&end=f45ada695c0cefd9467ffed78545a63ef2341bbd

view this post on Zulip cjgillot (Feb 10 2021 at 22:42):

Perf regression ~6%, a bit worse than the original IndexVec version. No significant difference on max-rss.


Last updated: Oct 21 2021 at 20:03 UTC