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

[BAD] refactor(gnovm): handle assignment operations in preprocess #1625

Closed
wants to merge 2 commits into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Feb 2, 2024

This was an attempt at refactoring the GnoVM code and simplify AssignStmt to re-write these statements at preprocess instead of during execution. Turns out that the expansion is significant and in benchmarks which contain the += operator, it slows down by up to 10%. See #1624 for the methodology used.

goos: linux
goarch: amd64
pkg: github.com/gnolang/gno/gnovm/pkg/gnolang
cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
                                     │ /home/howl/throw/bench_master.txt │  /home/howl/throw/bench_branch.txt   │
                                     │              sec/op               │    sec/op     vs base                │
Benchdata/fib.gno_param:4-16                                30.64µ ±  9%   29.18µ ±  3%        ~ (p=0.063 n=10)
Benchdata/fib.gno_param:8-16                                224.7µ ±  9%   223.2µ ±  7%        ~ (p=0.684 n=10)
Benchdata/fib.gno_param:16-16                               10.79m ±  3%   10.42m ±  3%   -3.46% (p=0.015 n=10)
Benchdata/loop.gno-16                                       473.9n ± 11%   509.2n ± 10%        ~ (p=0.565 n=10)
Benchdata/matrix_mult.gno_param:3-16                        617.6µ ±  7%   682.0µ ±  4%  +10.43% (p=0.000 n=10)
Benchdata/matrix_mult.gno_param:4-16                        1.834m ±  4%   1.925m ±  8%   +4.97% (p=0.015 n=10)
Benchdata/matrix_mult.gno_param:5-16                        6.548m ±  4%   6.741m ±  4%        ~ (p=0.280 n=10)
Benchdata/matrix_mult.gno_param:6-16                        33.94m ±  3%   34.73m ± 10%        ~ (p=0.481 n=10)
geomean                                                     553.9µ         566.6µ         +2.28%

Note: there is some improvement for fib, which does not use += and uses OpExec (OpBody/ ...) very often, so it seems to benefit from the removed code. However, the improvement does not seem significant enough to justify the change.

Creating this to document the attempt, so if someone else tries this they have some prior literature :)

Note: it seems likely to me that in matrix_mult, the difference is lower at higher values of the parameter (this modifies the size of the square matrix) because there is much more time spent allocating and creating sub-matrices for the computationally expensive det, than there is in executing its += operations.

@thehowl thehowl closed this Feb 2, 2024
@thehowl thehowl deleted the dev/morgan/assign-preprocess branch February 2, 2024 11:12
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant