feat(gnovm): fill in 3779, the gc PR #3789
Conversation
github-actions
bot
added
📦 🤖 gnovm
🛠 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):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
ltzmaxwell
changed the title
| func (fv *FuncValue) VisitAssociated(vis Visitor) (stop bool) { | ||
| // visit captures | ||
| for _, tv := range fv.Captures { | ||
| stop = vis(tv.V) |
There was a problem hiding this comment.
I think closure should be visited even if it's likely redundant.
in the heap PR it is called Parent or ParentBlock.
There was a problem hiding this comment.
yes i think it's redundant, but will add it for integrity.
There was a problem hiding this comment.
it introduces a closure -> parent -> closure cycle. it's easy just not visit it, or make a recurse guard? thinking.
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
ltzmaxwell
force-pushed
the
dev/maxwell/gc2
branch
from
1175e5f to
ec0100e
Compare
github-actions
bot
added
🧾 package/realm
| // ----------------------------------------------- | ||
|
|
||
| func (pv *PackageValue) GetShallowSize() int64 { | ||
| return allocPackage |
There was a problem hiding this comment.
I think these should account for all typedvalues as well.
There was a problem hiding this comment.
That is, the TypedValue array size should be included in the block, even if each value itself might have its own allocation on top.
| } | ||
|
|
||
| func (b *Block) GetShallowSize() int64 { | ||
| return allocBlock |
There was a problem hiding this comment.
same here, see AllocateBlock.
| if av.Data != nil { | ||
| return allocArray + int64(len(av.Data)) | ||
| } else { | ||
| return allocArray |
There was a problem hiding this comment.
same here for the else clause.
| } | ||
|
|
||
| func (sv *StructValue) GetShallowSize() int64 { | ||
| return allocStruct |
| func (fv *FuncValue) GetShallowSize() int64 { | ||
| if fv.IsClosure { | ||
| return allocFunc | ||
| } |
There was a problem hiding this comment.
This seems wrong, should be allocFunc no matter what I think.
|
This PR because it implements GetShallowSize, we should correctly allocate upon GetObject(). store.go:477: The garbage collector should also iterate over al objects in cacheObjects and remove items that do not have the updated gc cycle, because these objects are no longer reachable. |
github-project-automation
bot
moved this from In Review
to Done
in 🧙♂️gno.land core team
This PR is trying to complete the work outlined in #3779 .
It completes the
visitlogic and introduces benchmarking and gas consumption tracking.