Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace bimap dependency with a more efficient pair of maps, and arena-allocate slices. #951

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Nov 30, 2022

This originally was a prerequisite for introducing some asymmetry (for finer-grained zombies) in the maps, which bimap made impossible because it guarantees 1:1 (even when using insert instead of insert_no_overwrite, what it does is it removes any entries related to either side first, then adds the new pair of entries).

But it turned into a refactor of its own, once I realized I can get rid of per-entry allocations, using the power of tcx.arena.dropless. (an untyped arena that allows allocating individual values, or whole slices, as long as they're !Drop - this works great because there is no "drop to remember", which reduces overhead and soundness complexity - and by using the slice feature, at the very least Vecs can be replaced, and even entire data-recursive trees could be arena-allocated, even if it sadly doesn't generalize to e.g. HashMap/BTreeMap)

I haven't profiled this but it might be a bit faster? We were being pretty wasteful before, esp. lookup_type would .clone() variants of SpirvType that had Vecs in them, whereas now it's merely copying &[T]s.

Probably best to review each commit separately, matching the refactor stages.

@eddyb eddyb requested a review from oisyn as a code owner November 30, 2022 04:29
@eddyb eddyb enabled auto-merge (rebase) November 30, 2022 04:31
@eddyb eddyb merged commit acb05d3 into EmbarkStudios:main Nov 30, 2022
@eddyb eddyb deleted the un-bimap branch November 30, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants