From bc2b313cfbe0c99bd3d6536e950b28742d5b6edb Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:27:05 -0600 Subject: [PATCH 1/3] Replace domain string with enum for AccountStorageMap This commit replaces domain string, such as "storage", "public", "private", etc. with domain enum (integer) as the key for AccountStorageMap. Also, this uses CBOR to store the domain integer in shortest form. This optimization further improves storage efficiency of domain payloads of PR 3664. --- interpreter/account_storagemap.go | 20 ++--- interpreter/account_storagemap_test.go | 104 +++++++++++++++++------- runtime/runtime_memory_metering_test.go | 6 +- runtime/runtime_test.go | 2 +- 4 files changed, 88 insertions(+), 44 deletions(-) diff --git a/interpreter/account_storagemap.go b/interpreter/account_storagemap.go index 7498ae14c..e6ec1f342 100644 --- a/interpreter/account_storagemap.go +++ b/interpreter/account_storagemap.go @@ -76,7 +76,7 @@ func NewAccountStorageMapWithRootID( // DomainExists returns true if the given domain exists in the account storage map. func (s *AccountStorageMap) DomainExists(domain common.StorageDomain) bool { - key := StringStorageMapKey(domain.Identifier()) + key := Uint64StorageMapKey(domain) exists, err := s.orderedMap.Has( key.AtreeValueCompare, @@ -99,7 +99,7 @@ func (s *AccountStorageMap) GetDomain( domain common.StorageDomain, createIfNotExists bool, ) *DomainStorageMap { - key := StringStorageMapKey(domain.Identifier()) + key := Uint64StorageMapKey(domain) storedValue, err := s.orderedMap.Get( key.AtreeValueCompare, @@ -135,7 +135,7 @@ func (s *AccountStorageMap) NewDomain( domainStorageMap := NewDomainStorageMap(gauge, s.orderedMap.Storage, s.orderedMap.Address()) - key := StringStorageMapKey(domain.Identifier()) + key := Uint64StorageMapKey(domain) existingStorable, err := s.orderedMap.Set( key.AtreeValueCompare, @@ -181,7 +181,7 @@ func (s *AccountStorageMap) setDomain( ) (existed bool) { interpreter.recordStorageMutation() - key := StringStorageMapKey(domain.Identifier()) + key := Uint64StorageMapKey(domain) existingValueStorable, err := s.orderedMap.Set( key.AtreeValueCompare, @@ -218,7 +218,7 @@ func (s *AccountStorageMap) setDomain( func (s *AccountStorageMap) removeDomain(interpreter *Interpreter, domain common.StorageDomain) (existed bool) { interpreter.recordStorageMutation() - key := StringStorageMapKey(domain.Identifier()) + key := Uint64StorageMapKey(domain) existingKeyStorable, existingValueStorable, err := s.orderedMap.Remove( key.AtreeValueCompare, @@ -236,7 +236,7 @@ func (s *AccountStorageMap) removeDomain(interpreter *Interpreter, domain common // Key - // NOTE: Key is just an atree.Value (StringAtreeValue), not an interpreter.Value, + // NOTE: Key is just an atree.Value (Uint64AtreeValue), not an interpreter.Value, // so do not need (can) convert and not need to deep remove interpreter.RemoveReferencedSlab(existingKeyStorable) @@ -334,13 +334,13 @@ func (i *AccountStorageMapIterator) Next() (common.StorageDomain, *DomainStorage } func convertKeyToDomain(v atree.Value) common.StorageDomain { - key, ok := v.(StringAtreeValue) + key, ok := v.(Uint64AtreeValue) if !ok { panic(errors.NewUnexpectedError("domain key type %T isn't expected", key)) } - domain, found := common.StorageDomainFromIdentifier(string(key)) - if !found { - panic(errors.NewUnexpectedError("domain key %s isn't expected", key)) + domain, err := common.StorageDomainFromUint64(uint64(key)) + if err != nil { + panic(errors.NewUnexpectedError("domain key %d isn't expected: %w", key, err)) } return domain } diff --git a/interpreter/account_storagemap_test.go b/interpreter/account_storagemap_test.go index b926f2724..6a8794a60 100644 --- a/interpreter/account_storagemap_test.go +++ b/interpreter/account_storagemap_test.go @@ -20,6 +20,7 @@ package interpreter_test import ( "math/rand" + goruntime "runtime" "slices" "strconv" "strings" @@ -188,6 +189,8 @@ func TestAccountStorageMapCreateDomain(t *testing.T) { require.NotNil(t, accountStorageMap) require.Equal(t, uint64(0), accountStorageMap.Count()) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + for _, domain := range common.AllStorageDomains { const createIfNotExists = true domainStoragemap := accountStorageMap.GetDomain(nil, inter, domain, createIfNotExists) @@ -199,7 +202,12 @@ func TestAccountStorageMapCreateDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) t.Run("non-empty", func(t *testing.T) { @@ -225,6 +233,8 @@ func TestAccountStorageMapCreateDomain(t *testing.T) { const count = 10 accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + for _, domain := range common.AllStorageDomains { const createIfNotExists = true domainStoragemap := accountStorageMap.GetDomain(nil, inter, domain, createIfNotExists) @@ -238,7 +248,12 @@ func TestAccountStorageMapCreateDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) } @@ -271,6 +286,8 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) { require.NotNil(t, accountStorageMap) require.Equal(t, uint64(0), accountStorageMap.Count()) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + const count = 10 for _, domain := range common.AllStorageDomains { @@ -285,7 +302,12 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) t.Run("non-empty", func(t *testing.T) { @@ -311,6 +333,8 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) { const count = 10 accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + for _, domain := range common.AllStorageDomains { domainStorageMap := interpreter.NewDomainStorageMap(nil, storage, atree.Address(address)) @@ -324,7 +348,12 @@ func TestAccountStorageMapSetAndUpdateDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) } @@ -355,6 +384,8 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) { require.NotNil(t, accountStorageMap) require.Equal(t, uint64(0), accountStorageMap.Count()) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + for _, domain := range common.AllStorageDomains { existed := accountStorageMap.WriteDomain(inter, domain, nil) require.False(t, existed) @@ -362,7 +393,12 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) t.Run("non-empty", func(t *testing.T) { @@ -388,6 +424,8 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) { const count = 10 accountStorageMap, accountValues := createAccountStorageMap(storage, inter, address, existingDomains, count, random) + accountStorageMapRootSlabID := accountStorageMap.SlabID() + for _, domain := range common.AllStorageDomains { existed := accountStorageMap.WriteDomain(inter, domain, nil) @@ -398,7 +436,12 @@ func TestAccountStorageMapRemoveDomain(t *testing.T) { checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMapRootSlabID}) + + err := storage.PersistentSlabStorage.FastCommit(goruntime.NumCPU()) + require.NoError(t, err) + + checkAccountStorageMapDataWithRawData(t, ledger.StoredValues, ledger.StorageIndices, accountStorageMapRootSlabID, accountValues) }) } @@ -575,18 +618,7 @@ func TestAccountStorageMapLoadFromRootSlabID(t *testing.T) { accountStorageMapRootSlabID, accountValues, storedValues, storageIndices := init() - ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices) - storage := runtime.NewStorage(ledger, nil) - - accountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, accountStorageMapRootSlabID) - require.Equal(t, uint64(0), accountStorageMap.Count()) - require.Equal(t, accountStorageMapRootSlabID, accountStorageMap.SlabID()) - - inter := NewTestInterpreterWithStorage(t, storage) - - checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + checkAccountStorageMapDataWithRawData(t, storedValues, storageIndices, accountStorageMapRootSlabID, accountValues) }) t.Run("non-empty", func(t *testing.T) { @@ -619,18 +651,7 @@ func TestAccountStorageMapLoadFromRootSlabID(t *testing.T) { accountStorageMapRootSlabID, accountValues, storedValues, storageIndices := init() - ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices) - storage := runtime.NewStorage(ledger, nil) - - accountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, accountStorageMapRootSlabID) - require.Equal(t, uint64(len(existingDomains)), accountStorageMap.Count()) - require.Equal(t, accountStorageMapRootSlabID, accountStorageMap.SlabID()) - - inter := NewTestInterpreterWithStorage(t, storage) - - checkAccountStorageMapData(t, inter, accountStorageMap, accountValues) - - CheckAtreeStorageHealth(t, storage, []atree.SlabID{accountStorageMap.SlabID()}) + checkAccountStorageMapDataWithRawData(t, storedValues, storageIndices, accountStorageMapRootSlabID, accountValues) }) } @@ -697,6 +718,29 @@ func writeRandomValuesToDomainStorageMap( return domainValues } +// checkAccountStorageMapDataWithRawData checks loaded account storage map against expected account values. +func checkAccountStorageMapDataWithRawData( + tb testing.TB, + storedValues map[string][]byte, + storageIndices map[string]uint64, + rootSlabID atree.SlabID, + expectedAccountValues accountStorageMapValues, +) { + // Create new storage from raw data + ledger := NewTestLedgerWithData(nil, nil, storedValues, storageIndices) + storage := runtime.NewStorage(ledger, nil) + + inter := NewTestInterpreterWithStorage(tb, storage) + + loadedAccountStorageMap := interpreter.NewAccountStorageMapWithRootID(storage, rootSlabID) + require.Equal(tb, uint64(len(expectedAccountValues)), loadedAccountStorageMap.Count()) + require.Equal(tb, rootSlabID, loadedAccountStorageMap.SlabID()) + + checkAccountStorageMapData(tb, inter, loadedAccountStorageMap, expectedAccountValues) + + CheckAtreeStorageHealth(tb, storage, []atree.SlabID{rootSlabID}) +} + // checkAccountStorageMapData iterates account storage map and compares values with given expectedAccountValues. func checkAccountStorageMapData( tb testing.TB, diff --git a/runtime/runtime_memory_metering_test.go b/runtime/runtime_memory_metering_test.go index 09dc1d806..a141120e9 100644 --- a/runtime/runtime_memory_metering_test.go +++ b/runtime/runtime_memory_metering_test.go @@ -1073,7 +1073,7 @@ func TestRuntimeMeterEncoding(t *testing.T) { ) require.NoError(t, err) - assert.Equal(t, 114, int(meter.getMemory(common.MemoryKindBytes))) + assert.Equal(t, 107, int(meter.getMemory(common.MemoryKindBytes))) }) t.Run("string in loop", func(t *testing.T) { @@ -1122,7 +1122,7 @@ func TestRuntimeMeterEncoding(t *testing.T) { ) require.NoError(t, err) - assert.Equal(t, 61501, int(meter.getMemory(common.MemoryKindBytes))) + assert.Equal(t, 61494, int(meter.getMemory(common.MemoryKindBytes))) }) t.Run("composite", func(t *testing.T) { @@ -1173,6 +1173,6 @@ func TestRuntimeMeterEncoding(t *testing.T) { ) require.NoError(t, err) - assert.Equal(t, 58369, int(meter.getMemory(common.MemoryKindBytes))) + assert.Equal(t, 58362, int(meter.getMemory(common.MemoryKindBytes))) }) } diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index ce4c2219b..702def304 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -7579,7 +7579,7 @@ func TestRuntimeComputationMetring(t *testing.T) { `, ok: true, hits: 3, - intensity: 115, + intensity: 108, }, } From 88c31e68aa7032b2df7f357015ffb39de0701edb Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:24:41 -0600 Subject: [PATCH 2/3] Rename function convertKeyToDomain This commit renames function convertKeyToDomain to convertAccountStorageMapKeyToStorageDomain. --- interpreter/account_storagemap.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/interpreter/account_storagemap.go b/interpreter/account_storagemap.go index e6ec1f342..d06aaafe8 100644 --- a/interpreter/account_storagemap.go +++ b/interpreter/account_storagemap.go @@ -284,7 +284,7 @@ func (s *AccountStorageMap) Domains() map[common.StorageDomain]struct{} { break } - domain := convertKeyToDomain(k) + domain := convertAccountStorageMapKeyToStorageDomain(k) domains[domain] = struct{}{} } @@ -326,14 +326,14 @@ func (i *AccountStorageMapIterator) Next() (common.StorageDomain, *DomainStorage return common.StorageDomainUnknown, nil } - key := convertKeyToDomain(k) + key := convertAccountStorageMapKeyToStorageDomain(k) value := NewDomainStorageMapWithAtreeValue(v) return key, value } -func convertKeyToDomain(v atree.Value) common.StorageDomain { +func convertAccountStorageMapKeyToStorageDomain(v atree.Value) common.StorageDomain { key, ok := v.(Uint64AtreeValue) if !ok { panic(errors.NewUnexpectedError("domain key type %T isn't expected", key)) From f68c5eb99bed0ef1a2cf0d36bb29db3415d95494 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:33:31 -0600 Subject: [PATCH 3/3] Add comments and warnings about StorageDomain --- common/storagedomain.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/common/storagedomain.go b/common/storagedomain.go index 108196cda..ac7b297da 100644 --- a/common/storagedomain.go +++ b/common/storagedomain.go @@ -24,6 +24,19 @@ import ( "github.com/onflow/cadence/errors" ) +// StorageDomain is used to store domain values on chain. +// +// !!! *WARNING* !!! +// +// Only add new StorageDomain by: +// - appending to the end. +// +// Only remove StorageDomain by: +// - replacing existing StorageDomain with a placeholder `_`. +// +// DO *NOT* REPLACE EXISTING STORAGEDOMAIN! +// DO *NOT* REMOVE EXISTING STORAGEDOMAIN! +// DO *NOT* INSERT NEW STORAGEDOMAIN IN BETWEEN! type StorageDomain uint8 const ( @@ -54,6 +67,8 @@ const ( // StorageDomainAccountCapability is the storage domain which // records active account capability controller IDs StorageDomainAccountCapability + + // Append new StorageDomain here (if needed). ) var AllStorageDomains = []StorageDomain{