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

feat: sync gc #2640

Closed
wants to merge 2 commits into from
Closed

feat: sync gc #2640

wants to merge 2 commits into from

Conversation

petar-dambovaliev
Copy link
Contributor

Use the runtime.SetFinalizer api to decrease the counter in the VM allocator when garbage is collected.
This is to address this

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 28, 2024
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 27.02703% with 27 lines in your changes missing coverage. Please review.

Project coverage is 54.98%. Comparing base (0e3c050) to head (987f9a9).
Report is 1 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/alloc.go 27.02% 22 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
- Coverage   54.99%   54.98%   -0.01%     
==========================================
  Files         595      595              
  Lines       79775    79805      +30     
==========================================
+ Hits        43872    43884      +12     
- Misses      32581    32598      +17     
- Partials     3322     3323       +1     
Flag Coverage Δ
contribs/gnodev 26.00% <ø> (ø)
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.15% <ø> (ø)
gnovm 60.15% <27.02%> (-0.06%) ⬇️
misc/autocounterd 0.00% <ø> (ø)
misc/genproto 0.00% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/goscan 0.00% <ø> (ø)
misc/logos 17.38% <ø> (ø)
misc/loop 0.00% <ø> (ø)
tm2 54.50% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gfanton gfanton marked this pull request as ready for review July 28, 2024 21:50
@gfanton gfanton marked this pull request as draft July 28, 2024 21:50
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

I like the simplicity of this approach. It would be good to add some benchmarks to see how this performs against the current implementation, using realms that do allocations of varying size and frequency. Also, if alloc.bytes can now be modified from GC goroutines, should any other reads also be wrapped in atomic.LoadInt64?

@petar-dambovaliev
Copy link
Contributor Author

petar-dambovaliev commented Jul 29, 2024

I like the simplicity of this approach. It would be good to add some benchmarks to see how this performs against the current implementation, using realms that do allocations of varying size and frequency. Also, if alloc.bytes can now be modified from GC goroutines, should any other reads also be wrapped in atomic.LoadInt64?

Yes, any operation on alloc.bytes needs to be atomic, except the constructor.
I haven't covered all the locations its used. I am waiting for the validation of this approach.
@jaekwon

This way our allocator is in sync with the allocated and deallocated memory.
Both operations are performed at the same time.
If we want to hold off reducing the allocated bytes at the realm boundry,
it can easily be done by using the same API but adding another field to the allocator
that will accumulate deallocations and perform the subtraction at the end.

Also we need to define how much gas this would cost.

@petar-dambovaliev
Copy link
Contributor Author

In a meeting with Jae, he didn't like this approach. I'm closing this.

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
Development

Successfully merging this pull request may close these issues.

2 participants