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

fix: only executables should choose a global allocator #301

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented Jun 19, 2024

This is general rust wisdom, and it prevents me linking two different versions of evm_arithmetization in #275

@github-actions github-actions bot added crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Jun 19, 2024
@0xaatif
Copy link
Contributor Author

0xaatif commented Jun 19, 2024

FWIW I would normally want to see profiling before opting-in to any allocator change, but I don't want to get bogged down in that discussion for now

Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Sinilarly to your comment about opting-in a global allocator, I would expect profiling results before justifying removal. I believe Jacqui had good reasons bringing in the past and if there is a performance gain, and it is non-negligible, it may be worth considering alternative options?

Especially as there is little point in having both type1 / type2 live concurrently, hence the issues faced in the past by Brendan and possibly you now as well shouldn't be actually happening once we get to a more evolved state.

If we make it the default on zero-bin side then it should be fine though, but full removal should wait IMHO.

@0xaatif
Copy link
Contributor Author

0xaatif commented Jun 19, 2024

Thanks for the review,

Sinilarly to your comment about opting-in a global allocator, I would expect profiling results before justifying removal. I believe Jacqui had good reasons bringing in the past and if there is a performance gain, and it is non-negligible, it may be worth considering alternative options?

I don't think PR constitutes removal - it's changing who is specifying the global allocator (which is not changing) - perhaps your comment belongs in #302?

There's a bit of nuance in this PR: each executable has a global allocator, and I count 5 executables in the repo:

  • evm_arithmetization/src/bin/assemble.rs
  • zero_bin/leader/src/main.rs
  • zero_bin/rpc/src/main.rs
  • zero_bin/verifier/src/main.rs
  • zero_bin/worker/src/main.rs

I expect only worker to be on the allocation fast path, so that's the only one I've annotated - happy to be corrected on this though - I'll add #[global_allocator]s to the execs as needed.

To summarize:

  • we agree that we should do profiling before committing to/from an allocator.
  • we agree that executables should decide the allocator

    If we make it the default on zero-bin side then it should be fine though

  • we (may) disagree on which binaries opt-in to an allocator

What are the next steps to approve this PR?

@0xaatif 0xaatif merged commit 34e98cb into develop Jun 19, 2024
10 checks passed
@0xaatif 0xaatif deleted the 0xaatif/allocator branch June 19, 2024 12:05
@Nashtare
Copy link
Collaborator

I don't think PR constitutes removal - it's changing who is specifying the global allocator (which is not changing) - perhaps your comment belongs in #302?

Right it probably have been better suited within #302, I was just sharing concerns about removing jemalloc without further evidences it didn't bring significant savings, just for the benefits of "convenience".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants