-
Notifications
You must be signed in to change notification settings - Fork 444
feat(gnovm): garbage collector #4194
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
Conversation
... Readonly is a property of TypedValue, rather than ReadonlyValue wrapper fix tests Machine.IsReadonly() also checks realm mismatch nil interface is undefined fix IsReadonly to use IsImmutable; half of convert check done enable second opconvert check ... ... first commit of interrealm_withswitch fix zrealm_crossrealm11.gno test intermediate fix tests... fix gnovm tests ... heapitems_for_3693 update tests fix heap return issues ... cleanup and fixes heap item all package vars update golden tests a funcvalue is an object; a func closure has no parent; find heap captures fix update golden tests fix one example test update golden tests fix script tests add gno-memory-model replace IsUndefined(); remove defaultValue() for safety Add interrealm spec add note on closures update gno-interrealm.gno replace callerat with current/previous realm() borrrow realm -> switchrealm() test borrow test; fix tests fix gnolang#4019 fix stdlibs/std tests initial implementation of GC make it work save debug fixup save add todo package value size metric shallowly naive gas comsumption gc benchmark init fixup fixup fixup simplify fixup fixup txtar gas consumption fixup fmt fixup save save fixup revert benchmark change remove unused fix test add comment;update const;fix test fmt fixup revert clean fixup fixup fixup fixup string value based revert to array base clean fixup fixup correct string allocation fixup fix comment clear unused store gc callback revert string value fix after rebase optimize add comment
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
Linking the previous PR for comments: #3789 |
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
|
should also count |
gnovm/pkg/gnolang/alloc.go
Outdated
| // ----------------------------------------------- | ||
|
|
||
| func (pv *PackageValue) GetShallowSize() int64 { | ||
| return allocPackage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type PackageValue struct {
ObjectInfo
FBlocks []Value // Should we count this in the shallow size as well
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm technically yes, FNames and FBlocks, and their string sizes... we could do this here with _allocValue and _allocName, or do it in a separate PR. The strings we can re-count each time we see them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added here: 553a5e0
gnovm/pkg/gnolang/alloc.go
Outdated
|
|
||
| // Only count for closures. | ||
| func (fv *FuncValue) GetShallowSize() int64 { | ||
| return allocFunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type FuncValue struct {
ObjectInfo
Captures []TypedValue `json:",omitempty"` // Should we count this in the shallow size as well
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch—thank you for pointing that out. I agree it should be counted, but in the current implementation, these objects aren't allocated by the allocator (similar to m.Values).
We might optimize this later, to ensures the GC only recalculates previously allocated objects. otherwise the results would become inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that we don't account for these during doOpFuncLit(), but that's a separate issue.
We should still count these, and even assume that each one of these have inside a HeapItemValue for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah definitely. the reason I planned doing this (and the one above) after this PR is that, there potentially can be some weird behavior, that the GC calculation process exceeds the maxBytes itself, because it counts some extra objects sizes which are not allocated previously. of course, this is quite an unusual case.
it's still ok to do it here since luckily we already have the logic to handle this.
also agreed with the idea described below to ensure strict correctness.
|
I wish we could write something that collects GC'd objects with ref zero from realm finalization, e.g. into Alloc or some struct, and basically run GC to calculate sizes without actually removing anything; The purpose is to see whether the allocation bytes from runtime is actually identical to what is calculated during GC by adding both non-GC remaining objects and GC'd objects together. Whenever there is a difference in the numbers, means we are missing some allocation step, or the GC is not iterating on something. And then, we can also add tests to ensure that objects once put into the GC pile (those appended to Alloc for testing purposes as described in the previous paragraph), their GCCycle never changes, because the GC should never see them again. This should be its own Issue to work on after this gets merged; it could be something to work on after the beta launch. |
Yeah, assert that sum == remained + GC'd.
👍
OK. |
rebase to master. some issue to be resolved. --------- Co-authored-by: jaekwon <[email protected]>
rebase to master. some issue to be resolved. --------- Co-authored-by: jaekwon <[email protected]>
rebase to master. some issue to be resolved.