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: GC #2735

Closed
wants to merge 58 commits into from
Closed

feat: GC #2735

wants to merge 58 commits into from

Conversation

petar-dambovaliev
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev commented Aug 27, 2024

Deterministic garbage collector for gnovm.

Tracks allocations and deallocates objects that aren't used anymore.
It implements a simple mark and sweep garbage collector.
Related issue

Root objects (objects through which heap allocated objects are accessed) are identified in each scope and are released at the end of that scope.
When a heap allocated object has no roots, it is considered garbage and it is deallocated.

Maps are not implemented here.
For simplicity for reviewing and merging, they will be added in a following PR.

@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 27, 2024
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 84.88372% with 39 lines in your changes missing coverage. Please review.

Project coverage is 61.19%. Comparing base (5444859) to head (8325344).
Report is 85 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/alloc.go 68.60% 25 Missing and 2 partials ⚠️
gnovm/pkg/gnolang/gc.go 95.18% 2 Missing and 2 partials ⚠️
gnovm/pkg/gnolang/op_expressions.go 73.33% 3 Missing and 1 partial ⚠️
gno.land/pkg/sdk/vm/keeper.go 33.33% 2 Missing ⚠️
gnovm/pkg/gnolang/values.go 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2735      +/-   ##
==========================================
+ Coverage   61.10%   61.19%   +0.09%     
==========================================
  Files         564      566       +2     
  Lines       75355    75643     +288     
==========================================
+ Hits        46045    46293     +248     
- Misses      25946    25978      +32     
- Partials     3364     3372       +8     
Flag Coverage Δ
contribs/gnodev 61.46% <ø> (ø)
contribs/gnofaucet 15.31% <ø> (ø)
gno.land 67.93% <33.33%> (+<0.01%) ⬆️
gnovm 66.35% <85.49%> (+0.18%) ⬆️
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (ø)
tm2 62.03% <ø> (+0.04%) ⬆️

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.

@petar-dambovaliev petar-dambovaliev marked this pull request as ready for review September 2, 2024 20:52
@petar-dambovaliev petar-dambovaliev changed the title Feat: GC feat: GC Sep 3, 2024
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Oct 18, 2024
@ltzmaxwell
Copy link
Contributor

package main

func foo() {
	s := []int{1, 2, 3}
}
func main() {
	foo()
	s1 := []int{4, 5, 6}
}

in this one, should we also deallocate the underlying array as well?

m.Alloc.heap.RemoveRoot(obj.tv)
m.Alloc.DeallocatePointer()
}
} else if ho != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the usage of ho?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used only for the comparison, for convenience.

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Oct 20, 2024

I think the issue with this PR is that it's not "observable", there lacks some necessary tests. there should be a way to track what’s being allocated and deallocated, IMHO.

@petar-dambovaliev
Copy link
Contributor Author

petar-dambovaliev commented Oct 21, 2024

I think the issue with this PR is that it's not "observable", there lacks some necessary tests. there should be a way to track what’s being allocated and deallocated, IMHO.

What do you mean? Can you be more specific?
The GC managed objects have a field called marked, which is used to track that on the GC side.
On the allocator side, the user code that owns that allocation knows.

@ltzmaxwell
Copy link
Contributor

I think the issue with this PR is that it's not "observable", there lacks some necessary tests. there should be a way to track what’s being allocated and deallocated, IMHO.

What do you mean? Can you be more specific?

The current test focuses on gc.go, but it’s insufficient to reveal what’s happening under the hood when dealing with specific code. e.g.

package main

func foo() {
	s := []int{1, 2, 3}
}
func main() {
	foo()
	s1 := []int{4, 5, 6}
}

While the gc behavior is deterministic, it should be more observable. For instance, we could expose details like the current state of the allocator, metrics before and after memory allocation/deallocation, and more. What do you think?

@petar-dambovaliev
Copy link
Contributor Author

I think the issue with this PR is that it's not "observable", there lacks some necessary tests. there should be a way to track what’s being allocated and deallocated, IMHO.

What do you mean? Can you be more specific?

The current test focuses on gc.go, but it’s insufficient to reveal what’s happening under the hood when dealing with specific code. e.g.

package main

func foo() {
	s := []int{1, 2, 3}
}
func main() {
	foo()
	s1 := []int{4, 5, 6}
}

While the gc behavior is deterministic, it should be more observable. For instance, we could expose details like the current state of the allocator, metrics before and after memory allocation/deallocation, and more. What do you think?

The only way i could see this being viable is if its behind a debug flag.

@zivkovicmilos zivkovicmilos added this to the 🚀 Mainnet launch milestone Oct 28, 2024
@zivkovicmilos zivkovicmilos linked an issue Oct 29, 2024 that may be closed by this pull request
@moul moul requested review from ltzmaxwell and removed request for a team, moul, deelawn, mvertes, gfanton and zivkovicmilos November 5, 2024 16:13
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Needs a review from @jaekwon.

@Kouteki Kouteki requested review from thehowl, ltzmaxwell and mvertes and removed request for thehowl and ltzmaxwell November 15, 2024 09:19
@petar-dambovaliev
Copy link
Contributor Author

This approach was not approved by Jae so this PR is to be closed.

@Kouteki Kouteki removed the in focus Core team is prioritizing this work label Nov 29, 2024
@Gno2D2 Gno2D2 requested review from a team November 29, 2024 09:20
@gnolang gnolang deleted a comment from Gno2D2 Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[gno] synchronous garbage collection
7 participants