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.

Sign up for GitHub

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

[EN Performance] Reduce memory used for ledger.Payload by 32+ GB, eliminate 1+ billion allocs/op, speedup various ops #2930

Merged
merged 14 commits into from
Aug 10, 2022
Merged

[EN Performance] Reduce memory used for ledger.Payload by 32+ GB, eliminate 1+ billion allocs/op, speedup various ops #2930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
74ee21d
Resolve circular dependency
fxamacker Aug 2, 2022
b251754
Make ledger.Payload fields unexported
fxamacker Aug 3, 2022
e35bd94
Replace decoded payload key with encoded key buf
fxamacker Aug 4, 2022
9ae356c
Merge branch 'master' into fxamacker/use-encoded-key-in-payload
fxamacker Aug 4, 2022
5563418
Add comment for error
fxamacker Aug 4, 2022
a596875
Fix test
fxamacker Aug 4, 2022
83f4a13
Fix tests
fxamacker Aug 4, 2022
9a16971
Add custom JSON and CBOR encoding for Payload
fxamacker Aug 4, 2022
581a5d9
Fix lint error
fxamacker Aug 4, 2022
6af8b17
Add tests for ledger.Payload for JSON & CBOR
fxamacker Aug 5, 2022
a122da2
Add comment for ledger.Payload
fxamacker Aug 5, 2022
764c68b
Merge branch 'master' into fxamacker/use-encoded-key-in-payload
fxamacker Aug 5, 2022
ae5934a
Merge branch 'master' into fxamacker/use-encoded-key-in-payload
fxamacker Aug 9, 2022
209f3ea
Merge branch 'master' into fxamacker/use-encoded-key-in-payload
fxamacker Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions cmd/util/ledger/migrations/account_status_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,12 @@ func (as *AccountStatusMigration) Migrate(payload []ledger.Payload) ([]ledger.Pa
newPayloads := make([]ledger.Payload, 0, len(payload))

for _, p := range payload {
owner := p.Key.KeyParts[0].Value
key := p.Key.KeyParts[2].Value
k, err := p.Key()
if err != nil {
return nil, err
}
owner := k.KeyParts[0].Value
key := k.KeyParts[2].Value
Comment on lines +77 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some inherent structure to KeyParts? Maybe we can convert it to either a Struct or an Array to remove even more dynamic allocations?

Copy link
Member Author

@fxamacker fxamacker Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@SaveTheRbtz

Is there some inherent structure to KeyParts? Maybe we can convert it to either a Struct or an Array to remove even more dynamic allocations?

You're right, the structure of ledger.Key can be optimized to reduce number of allocs but it probably won't be worth the effort to do that after I eliminate use of ledger.Key to just migration and reporting.

Ledger key is designed to be flexible, so it can contain variable number of KeyPart. But we have plans to limit uses of Ledger key to migration and reports so that we can reduce number of heap allocs.

This PR eliminates ledger.Key from mtrie leaf nodes' payload.

My next PR related to ledger.Key is to eliminate it being created outside of migration and reporting which would reduce number of heap allocs caused by ledger.Key.

EDIT: add a one-sentence summary to the beginning of my reply and highlight variable names.


switch string(key) {
case KeyExists:
Expand All @@ -81,29 +85,29 @@ func (as *AccountStatusMigration) Migrate(payload []ledger.Payload) ([]ledger.Pa
as.setStatus(owner, st)
case KeyPublicKeyCount:
// follow the original way of decoding the value
countInt := new(big.Int).SetBytes(p.Value)
countInt := new(big.Int).SetBytes(p.Value())
count := countInt.Uint64()
// update status
status := as.getOrInitStatus(owner)
status.SetPublicKeyCount(count)
as.setStatus(owner, status)
case KeyStorageUsed:
// follow the original way of decoding the value
if len(p.Value) < 8 {
return nil, fmt.Errorf("malsized storage used, owner: %s value: %s", hex.EncodeToString(owner), hex.EncodeToString(p.Value))
if len(p.Value()) < 8 {
return nil, fmt.Errorf("malsized storage used, owner: %s value: %s", hex.EncodeToString(owner), hex.EncodeToString(p.Value()))
}
used := binary.BigEndian.Uint64(p.Value[:8])
used := binary.BigEndian.Uint64(p.Value()[:8])
// update status
status := as.getOrInitStatus(owner)
status.SetStorageUsed(used)
as.setStatus(owner, status)
case KeyStorageIndex:
// follow the original way of decoding the value
if len(p.Value) < 8 {
return nil, fmt.Errorf("malsized storage index, owner: %s value: %s", hex.EncodeToString(owner), hex.EncodeToString(p.Value))
if len(p.Value()) < 8 {
return nil, fmt.Errorf("malsized storage index, owner: %s value: %s", hex.EncodeToString(owner), hex.EncodeToString(p.Value()))
}
var index atree.StorageIndex
copy(index[:], p.Value[:8])
copy(index[:], p.Value()[:8])
// update status
status := as.getOrInitStatus(owner)
status.SetStorageIndex(index)
Expand Down
44 changes: 31 additions & 13 deletions cmd/util/ledger/migrations/account_status_migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,33 @@ func TestAccountStatusMigration(t *testing.T) {
address2 := flow.HexToAddress("0x2")

payloads := []ledger.Payload{
{Key: createPayloadKeyWithLegacyController(address1, KeyStorageUsed, true), Value: utils.Uint64ToBinary(12)},
{Key: createPayloadKeyWithLegacyController(address1, "other registers", true), Value: utils.Uint64ToBinary(2)},
{Key: createPayloadKeyWithLegacyController(address2, "other registers2", true), Value: utils.Uint64ToBinary(3)},
{Key: createPayloadKeyWithLegacyController(address1, KeyExists, true), Value: []byte{1}},
{Key: createPayloadKeyWithLegacyController(address1, KeyAccountFrozen, true), Value: []byte{1}},
{Key: createPayloadKeyWithLegacyController(address1, KeyPublicKeyCount, true), Value: utils.Uint64ToBinary(2)},
{Key: createPayloadKeyWithLegacyController(address1, KeyPrefixPublicKey+"0", true), Value: []byte{1}},
{Key: createPayloadKeyWithLegacyController(address1, KeyPrefixPublicKey+"1", true), Value: []byte{2}},
{Key: createPayloadKeyWithLegacyController(address1, KeyStorageIndex, true), Value: []byte{1, 0, 0, 0, 0, 0, 0, 0}},
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyStorageUsed, true),
utils.Uint64ToBinary(12)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, "other registers", true),
utils.Uint64ToBinary(2)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address2, "other registers2", true),
utils.Uint64ToBinary(3)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyExists, true),
[]byte{1}),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyAccountFrozen, true),
[]byte{1}),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyPublicKeyCount, true),
utils.Uint64ToBinary(2)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyPrefixPublicKey+"0", true),
[]byte{1}),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyPrefixPublicKey+"1", true),
[]byte{2}),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyStorageIndex, true),
[]byte{1, 0, 0, 0, 0, 0, 0, 0}),
}

newPayloads, err := mig.Migrate(payloads)
Expand All @@ -46,10 +64,10 @@ func TestAccountStatusMigration(t *testing.T) {
expectedStatus.SetPublicKeyCount(2)
expectedStatus.SetStorageUsed(12)
expectedStatus.SetStorageIndex([8]byte{1, 0, 0, 0, 0, 0, 0, 0})
expectedPayload := &ledger.Payload{
Key: createPayloadKeyWithLegacyController(address1, state.KeyAccountStatus, true),
Value: expectedStatus.ToBytes(),
}
expectedPayload := ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, state.KeyAccountStatus, true),
expectedStatus.ToBytes(),
)

// check address two status
require.True(t, newPayloads[4].Equals(expectedPayload))
Expand Down
13 changes: 9 additions & 4 deletions cmd/util/ledger/migrations/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,14 @@ func (l *led) Set(owner, key string, value flow.RegisterValue) error {
keyparts := []ledger.KeyPart{ledger.NewKeyPart(0, []byte(owner)),
ledger.NewKeyPart(2, []byte(key))}
fk := fullKey(owner, key)
l.payloads[fk] = ledger.Payload{Key: ledger.NewKey(keyparts), Value: ledger.Value(value)}
l.payloads[fk] = *ledger.NewPayload(ledger.NewKey(keyparts), ledger.Value(value))
return nil
}

func (l *led) Get(owner, key string) (flow.RegisterValue, error) {
fk := fullKey(owner, key)
return flow.RegisterValue(l.payloads[fk].Value), nil
p := l.payloads[fk]
return flow.RegisterValue(p.Value()), nil
}

func (l *led) Delete(owner, key string) error {
Expand All @@ -122,8 +123,12 @@ func (l *led) Payloads() []ledger.Payload {
func newLed(payloads []ledger.Payload) *led {
mapping := make(map[string]ledger.Payload)
for _, p := range payloads {
fk := fullKey(string(p.Key.KeyParts[0].Value),
string(p.Key.KeyParts[1].Value))
k, err := p.Key()
if err != nil {
panic(err)
}
fk := fullKey(string(k.KeyParts[0].Value),
string(k.KeyParts[1].Value))
mapping[fk] = p
}

Expand Down
12 changes: 8 additions & 4 deletions cmd/util/ledger/migrations/legacy_controller_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@ type LegacyControllerMigration struct {
func (lc *LegacyControllerMigration) Migrate(payload []ledger.Payload) ([]ledger.Payload, error) {
newPayloads := make([]ledger.Payload, len(payload))
for i, p := range payload {
owner := p.Key.KeyParts[0].Value
controller := p.Key.KeyParts[1].Value
key := p.Key.KeyParts[2].Value
k, err := p.Key()
if err != nil {
return nil, err
}
owner := k.KeyParts[0].Value
controller := k.KeyParts[1].Value
key := k.KeyParts[2].Value

if len(controller) > 0 {
if bytes.Equal(owner, controller) &&
Expand All @@ -43,7 +47,7 @@ func (lc *LegacyControllerMigration) Migrate(payload []ledger.Payload) ([]ledger
ledger.NewKeyPart(state.KeyPartOwner, owner),
ledger.NewKeyPart(state.KeyPartKey, key),
})
newPayloads[i] = *ledger.NewPayload(newKey, p.Value)
newPayloads[i] = *ledger.NewPayload(newKey, p.Value())
}
return newPayloads, nil
}
24 changes: 18 additions & 6 deletions cmd/util/ledger/migrations/legacy_controller_migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,21 @@ func TestLegacyControllerMigration(t *testing.T) {
address2 := flow.HexToAddress("0x2")

payloads := []ledger.Payload{
{Key: createPayloadKeyWithLegacyController(address1, KeyStorageUsed, false), Value: utils.Uint64ToBinary(1)},
{Key: createPayloadKeyWithLegacyController(address1, fvmstate.ContractKey("CoreContract"), true), Value: utils.Uint64ToBinary(2)},
{Key: createPayloadKeyWithLegacyController(address1, fvmstate.KeyContractNames, true), Value: utils.Uint64ToBinary(3)},
{Key: createPayloadKeyWithLegacyController(address2, fvmstate.KeyPublicKey(1), true), Value: utils.Uint64ToBinary(4)},
{Key: createPayloadKeyWithLegacyController(address2, KeyPublicKeyCount, true), Value: utils.Uint64ToBinary(4)},
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, KeyStorageUsed, false),
utils.Uint64ToBinary(1)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, fvmstate.ContractKey("CoreContract"), true),
utils.Uint64ToBinary(2)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address1, fvmstate.KeyContractNames, true),
utils.Uint64ToBinary(3)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address2, fvmstate.KeyPublicKey(1), true),
utils.Uint64ToBinary(4)),
*ledger.NewPayload(
createPayloadKeyWithLegacyController(address2, KeyPublicKeyCount, true),
utils.Uint64ToBinary(4)),
}

expectedKeys := []ledger.Key{
Expand All @@ -73,7 +83,9 @@ func TestLegacyControllerMigration(t *testing.T) {
require.Equal(t, len(payloads), len(newPayloads))

for i, p := range newPayloads {
require.Equal(t, expectedKeys[i], p.Key)
k, err := p.Key()
require.NoError(t, err)
require.Equal(t, expectedKeys[i], k)
}

}
2 changes: 1 addition & 1 deletion cmd/util/ledger/migrations/prune_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
func PruneMigration(payload []ledger.Payload) ([]ledger.Payload, error) {
newPayload := make([]ledger.Payload, 0, len(payload))
for _, p := range payload {
if len(p.Value) > 0 {
if len(p.Value()) > 0 {
newPayload = append(newPayload, p)
}
}
Expand Down
16 changes: 10 additions & 6 deletions cmd/util/ledger/migrations/storage_fees_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,23 @@ func StorageFeesMigration(payload []ledger.Payload) ([]ledger.Payload, error) {
make([]byte, 8))
u = u + uint64(storageUsedByStorageUsed)

newPayload = append(newPayload, ledger.Payload{
Key: registerIDToKey(flow.RegisterID{
newPayload = append(newPayload, *ledger.NewPayload(
registerIDToKey(flow.RegisterID{
Owner: s,
Key: "storage_used",
}),
Value: utils.Uint64ToBinary(u),
})
utils.Uint64ToBinary(u),
))
}
return newPayload, nil
}

func incrementStorageUsed(p ledger.Payload, used map[string]uint64) error {
id, err := KeyToRegisterID(p.Key)
k, err := p.Key()
if err != nil {
return err
}
id, err := KeyToRegisterID(k)
if err != nil {
return err
}
Expand All @@ -59,5 +63,5 @@ func incrementStorageUsed(p ledger.Payload, used map[string]uint64) error {
func registerSize(id flow.RegisterID, p ledger.Payload) int {
address := flow.BytesToAddress([]byte(id.Owner))
key := id.Key
return fvm.RegisterSize(address, key, p.Value)
return fvm.RegisterSize(address, key, p.Value())
}
18 changes: 14 additions & 4 deletions cmd/util/ledger/migrations/storage_used_update_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,12 @@ func (m *StorageUsedUpdateMigration) Migrate(payload []ledger.Payload) ([]ledger
for i := 0; i < workerCount; i++ {
inputEG.Go(func() error {
for p := range payloadChan {
id, err := KeyToRegisterID(p.Payload.Key)
k, err := p.Payload.Key()
if err != nil {
log.Error().Err(err).Msg("error get payload key")
return err
}
id, err := KeyToRegisterID(k)
if err != nil {
log.Error().Err(err).Msg("error converting key to register ID")
return err
Expand Down Expand Up @@ -172,7 +177,12 @@ Loop:
return nil, fmt.Errorf(errStr)
}

id, err := KeyToRegisterID(p.Key)
k, err := p.Key()
if err != nil {
log.Error().Err(err).Msg("error get payload key")
return nil, err
}
id, err := KeyToRegisterID(k)
if err != nil {
log.Error().Err(err).Msg("error converting key to register ID")
return nil, err
Expand All @@ -181,7 +191,7 @@ Loop:
return nil, fmt.Errorf("this is not a status register")
}

status, err := state.AccountStatusFromBytes(p.Value)
status, err := state.AccountStatusFromBytes(p.Value())
if err != nil {
log.Error().Err(err).Msg("error getting status")
return nil, err
Expand All @@ -202,7 +212,7 @@ Loop:
return nil, err
}
status.SetStorageUsed(used)
payload[pIndex].Value = status.ToBytes()
payload[pIndex] = *ledger.NewPayload(k, status.ToBytes())
}

m.Log.Info().
Expand Down
26 changes: 19 additions & 7 deletions cmd/util/ledger/migrations/storage_used_update_migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@ func TestStorageUsedUpdateMigrationMigration(t *testing.T) {
status.SetStorageUsed(1)
payload := []ledger.Payload{
// TODO (ramtin) add more registers
{Key: createAccountPayloadKey(address1, state2.KeyAccountStatus), Value: status.ToBytes()},
*ledger.NewPayload(
createAccountPayloadKey(address1, state2.KeyAccountStatus),
status.ToBytes(),
),
}
migratedPayload, err := mig.Migrate(payload)
require.NoError(t, err)

migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value)
migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value())
require.NoError(t, err)

require.Equal(t, len(migratedPayload), len(payload))
Expand All @@ -43,12 +46,15 @@ func TestStorageUsedUpdateMigrationMigration(t *testing.T) {
status := state2.NewAccountStatus()
status.SetStorageUsed(10000)
payload := []ledger.Payload{
{Key: createAccountPayloadKey(address1, state2.KeyAccountStatus), Value: status.ToBytes()},
*ledger.NewPayload(
createAccountPayloadKey(address1, state2.KeyAccountStatus),
status.ToBytes(),
),
}
migratedPayload, err := mig.Migrate(payload)
require.NoError(t, err)

migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value)
migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value())
require.NoError(t, err)

require.Equal(t, len(migratedPayload), len(payload))
Expand All @@ -59,12 +65,15 @@ func TestStorageUsedUpdateMigrationMigration(t *testing.T) {
status := state2.NewAccountStatus()
status.SetStorageUsed(40)
payload := []ledger.Payload{
{Key: createAccountPayloadKey(address1, state2.KeyAccountStatus), Value: status.ToBytes()},
*ledger.NewPayload(
createAccountPayloadKey(address1, state2.KeyAccountStatus),
status.ToBytes(),
),
}
migratedPayload, err := mig.Migrate(payload)
require.NoError(t, err)

migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value)
migratedStatus, err := state2.AccountStatusFromBytes(migratedPayload[0].Value())
require.NoError(t, err)

require.Equal(t, len(migratedPayload), len(payload))
Expand All @@ -73,7 +82,10 @@ func TestStorageUsedUpdateMigrationMigration(t *testing.T) {

t.Run("error is storage used does not exist", func(t *testing.T) {
payload := []ledger.Payload{
{Key: createAccountPayloadKey(address1, state2.KeyAccountStatus), Value: []byte{1}},
*ledger.NewPayload(
createAccountPayloadKey(address1, state2.KeyAccountStatus),
[]byte{1},
),
}
_, err := mig.Migrate(payload)
require.Error(t, err)
Expand Down
Loading