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

core/vm: specify concurrent tasks to 1 for bls msm precompiles #30358

Closed

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Aug 26, 2024

We've discussed that these should not be performed concurrently. Unfortunately, even with specifying NbTasks: 1, the implementation still uses goroutines.

Ultimately, it's unclear if we will use the current gnark MSM implementation, or there is some other MSM algorithm more amenable to single-threaded execution.

Overall, restricting NbTasks: 1 decreases performance of the MSM precompiles by ~40% in the worst case on my machine. Executing benchmarks with NbTasks: 1 becomes marginally better when GOMAXPROCS=1 is specified.

Performance for random inputs, ran on my M2 MBP (from the benchmarks I've put together here):

Edit: Made a mistake. re-running benchmarks and will post them again.

@holiman
Copy link
Contributor

holiman commented Sep 13, 2024

We've discussed that these should not be performed concurrently. Unfortunately, even with specifying NbTasks: 1, the implementation still uses goroutines.

Well, not necessarily. We might want to use multithreading. The important thing is that when benchmarking for gas pricing, we shouldn't allow it to use n goroutines. Because the gas shouldn't assume there are 8 available cores just idling on the processor, IMO.

AFAIK this also changes the gas values, which is currently ont in line with the bls spec? So this PR should not be merged until it's officially changed, right?

@jwasinger
Copy link
Contributor Author

jwasinger commented Sep 13, 2024

Removed the repricing. So this is mergeable if we do end up restricting to non-concurrent execution for MSM. With NbTasks: 1, the extraneous extra go-routines actually hurt performance (i.e. it's faster if I benchmark with GO_MAXPROCS=1).

@holiman
Copy link
Contributor

holiman commented Sep 13, 2024

Overall, restricting NbTasks: 1 decreases performance of the MSM precompiles by ~40%

I'm not sure why we should do this though?

@jwasinger
Copy link
Contributor Author

jwasinger commented Sep 13, 2024

To reasonably estimate the effect of keeping the code as-is, we would have to know the frequency (and input size) that these precompiles end up being called with on mainnet.

I can see a point in it probably being negligible enough (probably unmeasurable) that we can close this.

Striving for a puritan impl which reflects the gas schedule might not be worth intentionally hobbling our performance here.

@jwasinger jwasinger closed this Sep 16, 2024
This pull request was closed.
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