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

perf: use a sync pool for machine constructor #625

Merged
merged 6 commits into from
Mar 30, 2023
Merged

Conversation

peter7891
Copy link
Contributor

@peter7891 peter7891 commented Mar 20, 2023

This PR uses a sync.Pool in the Machine constructor.
This improves the following

  1. Reduces allocations and deallocations
  2. Improves runtime and memory consumption
  3. Because of less objects in memory, less GC pauses

issue with benchmarks from master.

Benchmarks on this branch.

CPU bench

Type: cpu
      10ms   100%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.(*Machine).PopAsPointer
         0     0%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.(*Machine).Run
         0     0%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.(*Machine).RunMain
         0     0%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.(*Machine).RunStatement
         0     0%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.(*Machine).doOpAddAssign
         0     0%   100%       10ms   100%  github.com/gnolang/gno/pkgs/gnolang.BenchmarkBlah
         0     0%   100%       10ms   100%  testing.(*B).launch
         0     0%   100%       10ms   100%  testing.(*B).runN

Memory profile

Showing nodes accounting for 10958.29kB, 72.79% of 15054.80kB total
Showing top 10 nodes out of 112
      flat  flat%   sum%        cum   cum%
 4097.83kB 27.22% 27.22%  4097.83kB 27.22%  github.com/gnolang/gno/pkgs/gnolang.addAssign
 1537.50kB 10.21% 37.43%  1537.50kB 10.21%  runtime.allocm
 1184.27kB  7.87% 45.30%  1184.27kB  7.87%  runtime/pprof.StartCPUProfile
  902.59kB  6.00% 51.29%  1553.21kB 10.32%  compress/flate.NewWriter
  650.62kB  4.32% 55.62%   650.62kB  4.32%  compress/flate.(*compressor).init
  528.17kB  3.51% 59.12%   528.17kB  3.51%  regexp.(*bitState).reset
  519.03kB  3.45% 62.57%   519.03kB  3.45%  time.init
  513.50kB  3.41% 65.98%   513.50kB  3.41%  github.com/cockroachdb/apd.(*Decimal).setString
  512.50kB  3.40% 69.39%   512.50kB  3.40%  github.com/gnolang/gno/pkgs/amino.(*Codec).parseStructInfoWLocked
  512.28kB  3.40% 72.79%  1024.29kB  6.80%  github.com/gnolang/gno/pkgs/gnolang.(*defaultStore).SetCacheType

As we can see, we no longer have the Machine constructor in the top 10 of either.
And we can see that the memory consumption went from 28666.37kB to 15054.80kB.

@peter7891 peter7891 requested a review from a team as a code owner March 20, 2023 22:57
@jaekwon
Copy link
Contributor

jaekwon commented Mar 21, 2023

Nice. What was used for benchmarking?

Added one comment for feedback above.

@peter7891
Copy link
Contributor Author

peter7891 commented Mar 21, 2023

Nice. What was used for benchmarking?

Added one comment for feedback above.

I ran the gno code like this, in the benchmark.

	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	m.RunMain()

And for the profiling, i used.

go tool pprof

@peter7891 peter7891 requested a review from jaekwon March 21, 2023 22:30
@peter7891 peter7891 requested a review from a team March 27, 2023 19:17
@moul
Copy link
Member

moul commented Mar 28, 2023

@jaekwon, do you think @peter7891's answer is satisfactory? Can we merge his work? Should we add any comments or issues for future improvements, or include unit tests?

@peter7891
Copy link
Contributor Author

peter7891 commented Mar 28, 2023

@jaekwon, do you think @peter7891's answer is satisfactory? Can we merge his work? Should we add any comments or issues for future improvements, or include unit tests?

The only thing i could possibly see adding a test for is to make sure the values from a fetched object are zero-ed.
I am not sure how valuable that is though. If that wasn't the case, all other functionality would break and thus other tests would break.
We can't detect if someone didn't release the object, which was @jaekwon concern.
So...

@peter7891 peter7891 changed the title use a sync pool for machine constructor perf: use a sync pool for machine constructor Mar 29, 2023
Copy link
Contributor

@jaekwon jaekwon left a comment

Choose a reason for hiding this comment

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

LGTM

@peter7891 peter7891 merged commit 41af8f6 into master Mar 30, 2023
@peter7891 peter7891 deleted the machine-sync-pool branch March 30, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants