-
Notifications
You must be signed in to change notification settings - Fork 447
feat(gnolang): add SaveTypes function to save types in cache when node starts #5008
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
base: master
Are you sure you want to change the base?
Conversation
🛠 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
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
i'll take a look by saturday |
jefft0
left a comment
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.
@Kouteki moved this to In Review and requested a review from core devs in a comment.
gnovm/pkg/gnolang/machine.go
Outdated
| // Save BlockNodes to m.Store. | ||
| SaveBlockNodes(m.Store, fn) | ||
|
|
||
| SaveTypes(m.Store, fn) |
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.
This shouldn't be necessary. Types are properly cached:
gno/gnovm/pkg/gnolang/machine.go
Lines 298 to 300 in e254b48
| if save { | |
| // store new package values and types | |
| throwaway = m.saveNewPackageValuesAndTypes() |
saveNewPackageValuesAndTypes calls SetType, and so the types should be stored.
(I noticed something weird that it may be worth investigating... and that is that only top-level types seem to be saved. If you have time, could you check whether types declared ie. inside of function bodies don't work across restarts especially?)
It seems to me that the cause is that a "cold" run would incur in the gas for GetType, while a "hot" run wouldn't (because it's already in the cache).
The solution is probably to lower the gas cost of GetType, but make it always paid regardless of whether it's cached or not.
This fix seems to work for the wrong reasons (it's actually doing unnecessary work on the database).
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.
This shouldn't be necessary. Types are properly cached:
But this part of the code is never called when the vm start, I am wrong ?
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.
SetCacheType? I'll see if that works instead.
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.
@thehowl: and that is that only top-level types seem to be saved. If you have time, could you check whether types declared ie. inside of function bodies don't work across restarts especially?)
I haven't checked empirically but pretty sure these are derived from preprocess and cached as AST node constTypeExpr so don't need to be in store.Cache. The only weird part I see is func (fv *FuncValue) GetType(store Store) has a case for RefType -> *FuncType but I'm checking to see if that ever happens.
This is because the preprocessor is run for all code and the types are already derived; but it's slower for saving because non-declared types need to be serialized for every TypedValue; and for loading because of the same reason. When we upgrade the chain in the future we can work on this so that the persisted types are appropriately RefTypes even for non-declared types.
I'm going to make a diff that does something slightly different than this PR. It will keep types in store.cacheTypes for efficiency but not just Type but TypeAndSize, and a second map for cacheTypesEphemeral (TypeID -> struct{}) that gets cleared at the end of the tx. Then later when we replace the cacheTypes map with a proper LRU type cache the logic that checks cacheTypesEphemeral is deterministic for every tx regardless of how cacheTypes is implemented, and regardless of restarts.
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.
But this part of the code is never called when the vm start, I am wrong ?
No, but the types are stored in the database, so they can be loaded from there (and that's why this PR has only an impact on gas rather than actual behaviour; because if data is in the store cache, loading it doesn't cost anything).
|
does the original issue still exist after #4925? |
I will say more this pr https://github.com/gnolang/gno/pull/2319/changes#diff-152fa445ed649a7f12eb067c6a67f7c6cb98c3242e23da200744b851aaeb3b20R173. The type cache for non uverse packages not being populated when node start. Consequently, a restarted node will ask more gas than one that hasn't been restarted. |
|
looking at this now |
|
@omarsy please check this diff. diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go
index e0676337c..244a6fca8 100644
--- a/gnovm/pkg/gnolang/machine.go
+++ b/gnovm/pkg/gnolang/machine.go
@@ -201,12 +201,10 @@ func (m *Machine) PreprocessAllFilesAndSaveBlockNodes() {
m.Store.SetBlockNode(pn)
PredefineFileSet(m.Store, pn, fset)
for _, fn := range fset.Files {
- // Save Types to m.Store (while preprocessing).
+ // Save Types to constTypeExpr (while preprocessing).
fn = Preprocess(m.Store, pn, fn).(*FileNode)
// Save BlockNodes to m.Store.
SaveBlockNodes(m.Store, fn)
-
- SaveTypes(m.Store, fn)
}
// Normally, the fileset would be added onto the
// package node only after runFiles(), but we cannot
diff --git a/gnovm/pkg/gnolang/store.go b/gnovm/pkg/gnolang/store.go
index 6cfa8f442..62fa16dc7 100644
--- a/gnovm/pkg/gnolang/store.go
+++ b/gnovm/pkg/gnolang/store.go
@@ -53,7 +53,7 @@ type Store interface {
DelObject(Object) int64 // returns size difference of the object
GetType(tid TypeID) Type
GetTypeSafe(tid TypeID) Type
- SetCacheType(Type)
+ SetCacheType(Type) // for builtin types only
SetType(Type)
GetPackageNode(pkgPath string) *PackageNode
GetBlockNode(Location) BlockNode
@@ -134,16 +134,22 @@ func DefaultGasConfig() GasConfig {
}
}
+type TypeAndSize struct {
+ Type Type
+ Size int
+}
+
type defaultStore struct {
// underlying stores used to keep data
baseStore store.Store // for objects, types, nodes
iavlStore store.Store // for escaped object hashes
// transaction-scoped
- cacheObjects map[ObjectID]Object // this is a real cache, reset with every transaction.
- cacheTypes txlog.Map[TypeID, Type] // this re-uses the parent store's.
- cacheNodes txlog.Map[Location, BlockNode] // until BlockNode persistence is implemented, this is an actual store.
- alloc *Allocator // for accounting for cached items
+ cacheObjects map[ObjectID]Object // this is a real cache, reset with every transaction.
+ cacheTypes txlog.Map[TypeID, TypeAndSize] // this re-uses the parent store's.
+ cacheTypesEph map[TypeID]Type // also gets cleared on ClearObjectCache.
+ cacheNodes txlog.Map[Location, BlockNode] // until BlockNode persistence is implemented, this is an actual store.
+ alloc *Allocator // for accounting for cached items
// Partially restored package; occupies memory and tracked for GC,
// this is more efficient than iterating over cacheObjects.
@@ -172,9 +178,10 @@ func NewStore(alloc *Allocator, baseStore, iavlStore store.Store) *defaultStore
alloc: alloc,
// cacheObjects is set; objects in the store will be copied over for any transaction.
- cacheObjects: make(map[ObjectID]Object),
- cacheTypes: txlog.GoMap[TypeID, Type](map[TypeID]Type{}),
- cacheNodes: txlog.GoMap[Location, BlockNode](map[Location]BlockNode{}),
+ cacheObjects: make(map[ObjectID]Object),
+ cacheTypes: txlog.GoMap[TypeID, TypeAndSize](map[TypeID]TypeAndSize{}),
+ cacheTypesEph: make(map[TypeID]Type),
+ cacheNodes: txlog.GoMap[Location, BlockNode](map[Location]BlockNode{}),
// reset at the message level
realmStorageDiffs: make(map[string]int64),
@@ -202,10 +209,11 @@ func (ds *defaultStore) BeginTransaction(baseStore, iavlStore store.Store, gasMe
iavlStore: iavlStore,
// transaction-scoped
- cacheObjects: make(map[ObjectID]Object),
- cacheTypes: txlog.Wrap(ds.cacheTypes),
- cacheNodes: txlog.Wrap(ds.cacheNodes),
- alloc: ds.alloc.Fork().Reset(),
+ cacheObjects: make(map[ObjectID]Object),
+ cacheTypes: txlog.Wrap(ds.cacheTypes),
+ cacheTypesEph: make(map[TypeID]Type),
+ cacheNodes: txlog.Wrap(ds.cacheNodes),
+ alloc: ds.alloc.Fork().Reset(),
// store configuration
pkgGetter: ds.pkgGetter,
@@ -231,7 +239,7 @@ type transactionStore struct {
}
func (t transactionStore) Write() {
- t.cacheTypes.(txlog.MapCommitter[TypeID, Type]).Commit()
+ t.cacheTypes.(txlog.MapCommitter[TypeID, TypeAndSize]).Commit()
t.cacheNodes.(txlog.MapCommitter[Location, BlockNode]).Commit()
}
@@ -764,11 +772,24 @@ func (ds *defaultStore) GetTypeSafe(tid TypeID) Type {
defer bm.ResumeOpCode()
}
- // check cache.
- if tt, exists := ds.cacheTypes.Get(tid); exists {
+ // check ephemeral cache.
+ tt, ephExists := ds.cacheTypesEph[tid]
+ if ephExists {
return tt
}
+ // check cache.
+ if tts, exists := ds.cacheTypes.Get(tid); exists {
+ // if not yet it ds.cacheTypesEph, consume gas.
+ if !ephExists {
+ gas := overflow.Mulp(ds.gasConfig.GasGetType, store.Gas(tts.Size))
+ ds.consumeGas(gas, GasGetTypeDesc)
+ ds.cacheTypesEph[tid] = tts.Type
+ }
+ // return type.
+ return tts.Type
+ }
+
// check backend.
if ds.baseStore != nil {
key := backendTypeKey(tid)
@@ -785,7 +806,8 @@ func (ds *defaultStore) GetTypeSafe(tid TypeID) Type {
}
}
// set in cache.
- ds.cacheTypes.Set(tid, tt)
+ ds.cacheTypes.Set(tid, TypeAndSize{tt, len(bz)})
+ ds.cacheTypesEph[tid] = tt
// after setting in cache, fill tt.
fillType(ds, tt)
return tt
@@ -794,15 +816,20 @@ func (ds *defaultStore) GetTypeSafe(tid TypeID) Type {
return nil
}
+// For builtin types only.
func (ds *defaultStore) SetCacheType(tt Type) {
tid := tt.TypeID()
- if tt2, exists := ds.cacheTypes.Get(tid); exists {
- if tt != tt2 {
+ // We do not set cacheTypesEph such that txs pay gas for these.
+ // // ds.cacheTypesEph[tid] = struct{}{}
+ if tts2, exists := ds.cacheTypes.Get(tid); exists {
+ if tt != tts2.Type {
panic(fmt.Sprintf("cannot re-register %q with different type", tid))
}
// else, already set.
} else {
- ds.cacheTypes.Set(tid, tt)
+ tcopy := copyTypeWithRefs(tt)
+ bz := amino.MustMarshalAny(tcopy)
+ ds.cacheTypes.Set(tid, TypeAndSize{tt, len(bz)})
}
}
@@ -820,9 +847,11 @@ func (ds *defaultStore) SetType(tt Type) {
}()
}
tid := tt.TypeID()
+ // first set ephemeral cache.
+ ds.cacheTypesEph[tid] = tt
// return if tid already known.
- if tt2, exists := ds.cacheTypes.Get(tid); exists {
- if tt != tt2 {
+ if tts2, exists := ds.cacheTypes.Get(tid); exists {
+ if tt != tts2.Type {
// this can happen for a variety of reasons.
// TODO classify them and optimize.
return
@@ -839,7 +868,7 @@ func (ds *defaultStore) SetType(tt Type) {
size = len(bz)
}
// save type to cache.
- ds.cacheTypes.Set(tid, tt)
+ ds.cacheTypes.Set(tid, TypeAndSize{tt, size})
}
// Convenience
@@ -1112,6 +1141,7 @@ func (ds *defaultStore) RealmStorageDiffs() map[string]int64 {
func (ds *defaultStore) ClearObjectCache() {
ds.alloc.Reset()
ds.cacheObjects = make(map[ObjectID]Object) // new cache.
+ ds.cacheTypesEph = make(map[TypeID]Type) // new cache.
ds.realmStorageDiffs = make(map[string]int64)
ds.opslog = nil // new ops log.
ds.SetCachePackage(Uverse())
@@ -1165,9 +1195,9 @@ func (ds *defaultStore) Print() {
utils.Print(ds.iavlStore)
fmt.Println(colors.Yellow("//----------------------------------------"))
fmt.Println(colors.Green("defaultStore:cacheTypes..."))
- ds.cacheTypes.Iterate()(func(tid TypeID, typ Type) bool {
+ ds.cacheTypes.Iterate()(func(tid TypeID, ts TypeAndSize) bool {
fmt.Printf("- %v: %v\n", tid,
- stringz.TrimN(fmt.Sprintf("%v", typ), 50))
+ stringz.TrimN(fmt.Sprintf("%v", ts.Type), 50))
return true
})
fmt.Println(colors.Yellow("//----------------------------------------"))
diff --git a/gnovm/pkg/gnolang/values.go b/gnovm/pkg/gnolang/values.go
index c328e40a6..2a1343c02 100644
--- a/gnovm/pkg/gnolang/values.go
+++ b/gnovm/pkg/gnolang/values.go
@@ -531,6 +531,9 @@ func (fv *FuncValue) GetType(store Store) *FuncType {
case nil:
return nil
case RefType:
+ // NOTE: This doesn't happen yet:
+ // only *DeclaredTypes become RefTypes.
+ // In the future we may change this.
typ := store.GetType(ct.ID).(*FuncType)
fv.Type = typ
return typand new gnovm/tests/files/zrealm_func.gno: // PKGPATH: gno.land/r/test
package test
var a any
func init() {
a = func(a int) {
}
}
func main(cur realm) {
a = func(b string) {
println("dontcare")
}
println("done")
}
// Output:
// done
// Realm:
// finalizerealm["gno.land/r/test"]
// c[a8ada09dee16d791fd406d629fe29bb0ed084a30:8](309)={
// "Crossing": false,
// "FileName": "",
// "IsClosure": false,
// "IsMethod": false,
// "Name": "",
// "NativeName": "",
// "NativePkg": "",
// "ObjectInfo": {
// "ID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:8",
// "LastObjectSize": "309",
// "ModTime": "0",
// "OwnerID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:3",
// "RefCount": "1"
// },
// "Parent": null,
// "PkgPath": "gno.land/r/test",
// "Source": {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "File": "zrealm_func.gno",
// "PkgPath": "gno.land/r/test",
// "Span": {
// "End": {
// "Column": "3",
// "Line": "14"
// },
// "Num": "0",
// "Pos": {
// "Column": "6",
// "Line": "12"
// }
// }
// }
// },
// "Type": {
// "@type": "/gno.FuncType",
// "Params": [
// {
// "Embedded": false,
// "Name": "b",
// "Tag": "",
// "Type": {
// "@type": "/gno.PrimitiveType",
// "value": "16"
// }
// }
// ],
// "Results": []
// }
// }
// u[a8ada09dee16d791fd406d629fe29bb0ed084a30:3](0)=
// @@ -2,7 +2,7 @@
// "ObjectInfo": {
// "ID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:3",
// "LastObjectSize": "351",
// - "ModTime": "6",
// + "ModTime": "7",
// "OwnerID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:2",
// "RefCount": "1"
// },
// @@ -12,11 +12,11 @@
// "Params": [
// {
// "Embedded": false,
// - "Name": "a",
// + "Name": "b",
// "Tag": "",
// "Type": {
// "@type": "/gno.PrimitiveType",
// - "value": "32"
// + "value": "16"
// }
// }
// ],
// @@ -24,8 +24,8 @@
// },
// "V": {
// "@type": "/gno.RefValue",
// - "Hash": "8806871058538d9ab7d4c168c6788e300f7e72fa",
// - "ObjectID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:7"
// + "Hash": "7b5d94f791350d9cbeab878b065cdf86f258c507",
// + "ObjectID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:8"
// }
// }
// }
// d[a8ada09dee16d791fd406d629fe29bb0ed084a30:7](-309)And can you please make a txtar for gno.land/pkgs/integration/testdata where, in one file, both from #4983 are tested, and the gas used are checked to be identical? If you cannot figure out a way to do this easily maybe we can keep them in one file with two outputs, with prominent comment in the file that the two gas outputs should be the same. See also #5008 (comment) |
|
@jaekwon This would be one of the crucial issues to sort out before launch, because otherwise nodes have to replay from the beginning to participate at any time. Please make sure to keep track of this |
ltzmaxwell
left a comment
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.
note as a first pass after skim read. it looks generally good to make gas cost for GetType consistent across transactions.
gnovm/pkg/gnolang/preprocess.go
Outdated
| func SaveTypes(store Store, fn *FileNode) { | ||
| Transcribe(fn, func(ns []Node, ftype TransField, index int, n Node, stage TransStage) (Node, TransCtrl) { | ||
| if stage != TRANS_ENTER { | ||
| return n, TRANS_CONTINUE | ||
| } | ||
|
|
||
| if typeDecl, ok := n.(*TypeDecl); ok { | ||
| store.SetType(getType(typeDecl.Type)) | ||
| } | ||
|
|
||
| return n, TRANS_CONTINUE | ||
| }) | ||
| } |
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.
this is not used anymore.
|
I made this PR as a simpler (as in, it actually removes code instead of adding more) fix: #5133 |
Co-authored-by: ltzmaxwell <[email protected]>
alternative #4984