-
Notifications
You must be signed in to change notification settings - Fork 75
perf(levm): use a stack pool #3386
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
Conversation
Lines of code reportTotal lines added: Detailed view
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 9a1c789Click to view benchmark
|
Benchmark for f8c5fb7Click to view benchmark
|
Benchmark for 7e78336Click to view benchmark
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 2df5e75Click to view benchmark
|
Benchmark for 20ddfbcClick to view benchmark
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 2822e71Click to view benchmark
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 54b8fd0Click to view benchmark
|
Benchmark for 8d2e632Click to view benchmark
|
Benchmark Block Execution Results Comparison Against Main
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for e3f4bfdClick to view benchmark
|
External benchmarks show an improved result |
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for b8d9533Click to view benchmark
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 64bf484Click to view benchmark
|
Benchmark for 07f7206Click to view benchmark
|
**Motivation** The fibonacci recursive can show perfomance results of stack reuse that the factorial recursive one can't because factorial will never be able to "reuse" the stack. See also #3386 **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for b6874ddClick to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s say a real transaction in ethereum that is “call heavy” will have at most 50 created callframes. Is the stack pool beneficial in that scenario or is the difference negligible? I’m asking because even though this change impacts very positively on the Recursive Fibonacci test I think that we should care more about real case scenarios, and if there’s no difference then maybe it’s not worth adding it.
The codebase is becoming larger by the day and maybe we should prioritize changes that affect performance in real transactions. Let me know what you think though.
Edit: This comment doesn't apply to this PR, I've seen that it has improved another benchmarks too so for me it's good to go.
**Motivation** The fibonacci recursive can show perfomance results of stack reuse that the factorial recursive one can't because factorial will never be able to "reuse" the stack. See also #3386 **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number
**Motivation** The fibonacci recursive can show perfomance results of stack reuse that the factorial recursive one can't because factorial will never be able to "reuse" the stack. See also #3386 **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: Push
|
Benchmark for 2198890Click to view benchmark
|
Motivation
Fixes #3385
https://share.firefox.dev/44uRnnn
The perfomance gain from this pr cannot be seen with the factorial recursive bench, because it doesn't reuse the stack, it always goes full deep and then up.
With a fibonacci recursive bench it can be seen:
Main

PR

Needs #3391 to show the perf gains in benches