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

chore(perf): try making AliasSet an enum #7057

Closed
wants to merge 3 commits into from

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Jan 14, 2025

Description

Problem

Related to #7001

Summary

While pairing with Akosh on improving compilation memory usage he mentioned that he found that most alias sets had one element in them. I was curious if instead of using a BTreeSet of size one using an enum variant for that case would improve memory usage a bit, so that's what this PR does.

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Execution Memory Report

Program Peak Memory
keccak256 74.67M
workspace 123.92M
regression_4709 316.00M
ram_blowup_regression 512.61M
rollup-base-public 478.87M
rollup-base-private 325.48M
private-kernel-tail 180.47M
private-kernel-reset 245.16M
private-kernel-inner 208.55M

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Compilation Memory Report

Program Peak Memory %
keccak256 77.91M -2%
workspace 123.88M 0%
regression_4709 424.09M 0%
ram_blowup_regression 1.58G 0%
rollup-base-public 3.50G -25%
rollup-base-private 989.55M 100%
private-kernel-tail 207.18M 0%
private-kernel-reset 669.26M 0%
private-kernel-inner 294.40M 0%

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Compilation Report

Program Compilation Time %
sha256_regression 1.210s 1%
regression_4709 0.859s 0%
ram_blowup_regression 17.100s 2%
rollup-root 3.336s -2%
rollup-merge 1.918s -3%
rollup-block-merge 3.442s 0%
rollup-base-public 38.520s -2%
rollup-base-private 11.620s -5%
private-kernel-tail 1.068s 13%
private-kernel-reset 6.424s -2%
private-kernel-inner 1.996s -2%

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Execution Report

Program Execution Time %
sha256_regression 0.052s -2%
regression_4709 0.001s 0%
ram_blowup_regression 0.601s -1%
rollup-root 0.104s -1%
rollup-merge 0.006s 0%
rollup-block-merge 0.104s 0%
rollup-base-public 1.242s 1%
rollup-base-private 0.452s 0%
private-kernel-tail 0.019s -6%
private-kernel-reset 0.313s 0%
private-kernel-inner 0.072s 5%

@TomAFrench
Copy link
Member

https://crates.io/crates/vec-collections could be useful here.

@asterite
Copy link
Collaborator Author

It's a bit strange because in another recent PR, for example #7056, we get this:

image

Here we get this:

image

The 100% increase doesn't make sense 🤔

@asterite asterite changed the title chore: try making AliasSet an enum perf: try making AliasSet an enum Jan 14, 2025
@asterite asterite changed the title perf: try making AliasSet an enum chore(perf): try making AliasSet an enum Jan 14, 2025
@asterite asterite marked this pull request as ready for review January 14, 2025 14:27
@TomAFrench
Copy link
Member

Those percentages have been a bit suss for a while, I tend to go off the raw figures generally.

I've got plans to move us off of that github action anyway.

@TomAFrench
Copy link
Member

@asterite Thoughts on https://crates.io/crates/vec-collections which could extend some of the benefits of this PR past a single alias? I haven't dug into the distribution of alias set sizes however.

@jfecher
Copy link
Contributor

jfecher commented Jan 14, 2025

@TomAFrench if anything wrapping SmallVec in those collections should lead to more memory usage, not less. Unless size_of::<SmallVec<T>>() <= size_of::<Vec<T>>() then we'll be using more space for smaller vecs even if they're allocating their data on the stack. I could be wrong but that's my guess.

I suppose if the sizes are similar we'd be saving space from not having an allocation header/metadata from the heap allocator.

@asterite
Copy link
Collaborator Author

Oh, when I read vec-collections and started reading the docs I thought it provided only vecs, when we need sets here, but I see they provide sets too. I'll try it in a separate PR to see what happens.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aakoshh
Copy link
Contributor

aakoshh commented Jan 14, 2025

This changes the memory profile two ways:

  1. It brings the memory used down to 3.5GB:
Screenshot 2025-01-14 at 14 42 27
  1. Shortens the duration of the mem2reg pass:
Screenshot 2025-01-14 at 14 44 43

Instead of a symmetrical pyramid as in the baseline of #7053 we get a steeper descent, and the whole thing lasts just seconds compared to 30s originally.

@TomAFrench
Copy link
Member

I'd need to think about it a little bit @jfecher. Good chance you're right and the fact we'd be putting it inside an enum would risk inflating the other variants.

@TomAFrench TomAFrench added this pull request to the merge queue Jan 14, 2025
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Jan 14, 2025
@aakoshh aakoshh mentioned this pull request Jan 14, 2025
5 tasks
@asterite asterite closed this Jan 14, 2025
@asterite asterite deleted the ab/mem2reg-alias-set-enum branch January 14, 2025 16:36
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.

4 participants