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

Investigate reducing the size of MastNode to 64 bytes #1490

Open
plafer opened this issue Sep 10, 2024 · 4 comments
Open

Investigate reducing the size of MastNode to 64 bytes #1490

plafer opened this issue Sep 10, 2024 · 4 comments

Comments

@plafer
Copy link
Contributor

plafer commented Sep 10, 2024

Originally posted by @bobbinth in #1466 (comment)

Using vectors here increases the size of this struct from 40 bytes to 88 bytes. This also means that the size of MastNode enum grows to 96 bytes. Our goal should be to reduce the size of the MastNode to 64 bytes or fewer - but we can probably do it in a separate PR.

This might involve introducing the DecoratorSpan idea mentioned in #1122 (comment).

@yasonk
Copy link
Contributor

yasonk commented Dec 11, 2024

@bobbinth @plafer In this comment , there is a proposal to use a Box type.

Benefits of using Box:

  1. A Box would only keep a pointer to the data on the stack (4 bytes or 8 bytes).
  2. It seems it would also result in more intuitive code for developers work on this part of code in the future.
  3. Maintain flexibility of having a Vector of Ids be independent of how the Decorators are actually stored.

Downsides of using Box:

  1. A little more space on the heap (probably negligible).
  2. Some performance overhead to copy the 16 or 24 bytes to heap. (probably negligible since this will happen only once)

If I'm not missing some use case, it would seem using Box would be even more efficient in reducing the size of the struct. What do you think?

@plafer
Copy link
Contributor Author

plafer commented Dec 11, 2024

Since then we learned that we want neither Box nor Vec - basically nothing that comes with an allocation. The current code (which uses Vec) is slowing down significantly some tests in the compiler (see #1497), and that turned out to be due to the massive amount of allocations/deallocations when using debug symbols.

@bitwalker proposes using an arena in there, but I haven't gotten back to it in a while, and it's still unclear to me the direction we want to go in.

@bobbinth
Copy link
Contributor

I think there are a couple of things here and we can probably approach them incrementally:

  1. First, we want to get rid of using vectors to hold decorator data in MAST nodes (e.g., before_enter and after_exit fields in the JoinNode etc. and decorators field in the BasicBlockNode). Allocating these vectors has a significant impact on performance (especially during testing). To get rid of this, the simplest approach is probably to introduce DecoratorSpan discussed in various comments.
  2. Then, we could pick off some "low-hanging fruit" for reducing the size of MastNode - e.g., using Box<[OpBatch]> instead of Vec<OpBatch> in the BasicBlockNode. This probably won't get us all the way to 64 bytes per node.
  3. Then, we can try to use a more sophisticated approach (e.g., something like arena) to reduce the memory footprint even further.

In may mind, the first 2 items will probably get us 80% of what we need pretty quickly, and the arena approach could be more of a long-term goal. So, I'd go for these two but maybe in different PRs.

@yasonk
Copy link
Contributor

yasonk commented Dec 12, 2024

Ok. Makes sense. I can give (1) a try

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

No branches or pull requests

3 participants