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

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

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 4 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 changed the title Feat: GC feat: GC Sep 3, 2024
gnovm/pkg/gnolang/alloc.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/machine.go Outdated Show resolved Hide resolved
@@ -141,6 +141,31 @@ func TestRunEmptyMain(t *testing.T) {
m.RunMain()
}

func TestGCLoopy(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect this to succeed or fail?

It failed with the error message panic: allocation limit exceed on my local machine

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 should pass. I just ran this test locally and it passes.
It also has passed in CI. This is strange that it fails on your machine.

m.Alloc = NewAllocator(22000, 3000, NewHeap())
m.RunMain()
}

// run main() with a for loop.
Copy link
Contributor

Choose a reason for hiding this comment

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

I updated ShouldRunGC() so that we can control when to trigger the GC.

func (alloc *Allocator) ShouldRunGC() bool {
	// return false
	return alloc.bytes >= alloc.minGcRun
}

For the following tests. regardless we triggered GC or not, Alloc.bytes are the same. Not sure if this is the expected behavior

run two simple GC deallocation test.

func TestGCAlloc(t *testing.T) {
	c := `package test
func simpleGC() {
	{
		item1 := make([]byte, 100)
	}
}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	//m.Alloc = NewAllocator(22000, 10000, NewHeap())
	m.Alloc = NewAllocator(22000, 1, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)

}

//&{maxBytes:22000 bytes:1140 lastGcRun:0 minGcRun:10000 heap:0x1400030bbc0 detonate:false}
//&{maxBytes:22000 bytes:1140 lastGcRun:2348 minGcRun:1 heap:0x14000341bc0 detonate:false}
func TestGCAlloc(t *testing.T) {
	c := `package test
func simpleGC() {
	{
		item1 := make([]byte, 100)
	}
}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	//m.Alloc = NewAllocator(22000, 10000, NewHeap())
	m.Alloc = NewAllocator(22000, 1, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)

}


// &{maxBytes:22000 bytes:1920 lastGcRun:2440 minGcRun:1 heap:0x14000330120 detonate:false}
// &{maxBytes:22000 bytes:1920 lastGcRun:0 minGcRun:10000 heap:0x1400035a120 detonate:false}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated ShouldRunGC() so that we can control when to trigger the GC.

func (alloc *Allocator) ShouldRunGC() bool {
	// return false
	return alloc.bytes >= alloc.minGcRun
}

For the following tests. regardless we triggered GC or not, Alloc.bytes are the same. Not sure if this is the expected behavior

run two simple GC deallocation test.

func TestGCAlloc(t *testing.T) {
	c := `package test
func simpleGC() {
	{
		item1 := make([]byte, 100)
	}
}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	//m.Alloc = NewAllocator(22000, 10000, NewHeap())
	m.Alloc = NewAllocator(22000, 1, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)

}

//&{maxBytes:22000 bytes:1140 lastGcRun:0 minGcRun:10000 heap:0x1400030bbc0 detonate:false}
//&{maxBytes:22000 bytes:1140 lastGcRun:2348 minGcRun:1 heap:0x14000341bc0 detonate:false}
func TestGCAlloc(t *testing.T) {
	c := `package test
func simpleGC() {
	{
		item1 := make([]byte, 100)
	}
}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	//m.Alloc = NewAllocator(22000, 10000, NewHeap())
	m.Alloc = NewAllocator(22000, 1, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)

}


// &{maxBytes:22000 bytes:1920 lastGcRun:2440 minGcRun:1 heap:0x14000330120 detonate:false}
// &{maxBytes:22000 bytes:1920 lastGcRun:0 minGcRun:10000 heap:0x1400035a120 detonate:false}

Jae said we should only run the GC if we run out of memory. This is why it was commented and always returned false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code you wrote does not reach the limit in memory so the GC is never triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, got it.

What about these two testing cases? Does the GC kick in? Is the result expected?

func TestGCAlloc(t *testing.T) {
	c := `package test
func simpleGC() {
	{
		item1 := make([]byte, 10000)

	}
	{
		item2 := make([]byte, 10000)

	}
}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	m.Alloc = NewAllocator(22000, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)

}

// panic: allocation limit exceeded

func TestGCAlloc2(t *testing.T) {
	c := `package test

func simpleGC() {

		for i:=0; i< 1000;i++{
			item1 := [10]int{}
		}

}`
	m := NewMachine("test", nil)
	n := MustParseFile("main.go", c)
	m.RunFiles(n)
	m.Alloc = NewAllocator(22000, NewHeap())
	m.Eval(Call("simpleGC"))
	fmt.Printf("%+v\n", m.Alloc)
	// panic: allocation limit exceeded

}
// panic: allocation limit exceeded

Copy link
Contributor Author

@petar-dambovaliev petar-dambovaliev Oct 16, 2024

Choose a reason for hiding this comment

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

Yes. These are expected because all the builtins and std are allocating too. So the allowed maximum allocation number needs to be increased. 22000 is not enough for builtins and std plus the stuff you are allocating in your code.

@petar-dambovaliev
Copy link
Contributor Author

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.

Good work!

some questions:

How are heap objects defined here related to objects in the Ownership Tree and 'real' objects in persistent storage? Could there be conflicts we need to check?

When we run filetest and gno test, does it actually trigger GC? Can we run the tests with a limited allocation threshold to trigger GC for all test cases, ensuring that even with GC involved, they still pass?

The way the allocator interacts with persistent storage, it shouldn't cause issues for the GC.

The GC runs in all tests.
At the moment, for filetests we use

maxAllocTx := int64(500 * 1000 * 1000)

We can reduce this now.

@petar-dambovaliev
Copy link
Contributor Author

@piux2 i reduced the max allocation for the tests by a factor of 10 and everything works. That means the GC is doing its job :)
There are 2 new failures which seem to be unrelated to this PR.
I'll need to investigate.

@petar-dambovaliev petar-dambovaliev requested a review from a team as a code owner September 30, 2024 01:28
alloc.AllocateStruct()
alloc.AllocateStructFields(int64(len(v.Fields)))
alloc.AllocateType()
alloc.AllocateHeapItem()
Copy link
Contributor

Choose a reason for hiding this comment

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

StructValue does not imply heapItemValue allocation, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code refers to StructValue as a result of a RefExpr.
The VM currently uses NewHeapItem for it.

	case *CompositeLitExpr: // for *RefExpr
		tv := *m.PopValue()
		hv := m.Alloc.NewHeapItem(tv)
		return PointerValue{
			TV:    &hv.Value,
			Base:  hv,
			Index: 0,
		}

alloc.DeallocateStruct()
alloc.DeallocateStructFields(int64(len(v.Fields)))
alloc.DeallocateType()
alloc.DeallocateHeapItem()
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 ⛰️ 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: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants