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.

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

[FVM] merge constant size account registers in account status #2799

Merged
merged 44 commits into from
Jul 19, 2022

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Jul 13, 2022

This PR

ramtinms and others added 30 commits June 27, 2022 20:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…thub.com:onflow/flow-go into ramtin/fvm-remove-legacy-controller-from-storage
…thub.com:onflow/flow-go into ramtin/fvm-remove-legacy-controller-from-storage
@ramtinms ramtinms marked this pull request as ready for review July 13, 2022 22:21
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 Just some minor comments.

fvm/state/accounts_status.go Outdated Show resolved Hide resolved

const (
maskExist byte = 0b0000_0001
maskFrozen byte = 0b1000_0000
)

// NewAccountStatus sets exist flag and return an AccountStatus
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove exist flag from the comment because it isn't used anymore.

Suggested change
// NewAccountStatus sets exist flag and return an AccountStatus
// NewAccountStatus returns a new AccountStatus


// SetStorageUsed updates the storage used by the account
func (a AccountStatus) SetStorageUsed(used uint64) {
binary.BigEndian.PutUint64(a[storageUsedStartIndex:storageIndexStartIndex], used)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this could be less dependent on element sequence.

Suggested change
binary.BigEndian.PutUint64(a[storageUsedStartIndex:storageIndexStartIndex], used)
binary.BigEndian.PutUint64(a[storageUsedStartIndex:storageUsedStartIndex+storageUsedSize], used)

fvm/state/accounts_status.go Outdated Show resolved Hide resolved
fvm/state/accounts_status.go Outdated Show resolved Hide resolved
}
return AccountStatus(uint8(inp) & (0xFF - maskFrozen))
a[0] = a[0] & (0xFF - maskFrozen)
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

Suggested change
a[0] = a[0] & (0xFF - maskFrozen)
a[flagIndex] = a[flagIndex] & (0xFF - maskFrozen)

fvm/state/accounts_status_test.go Outdated Show resolved Hide resolved
fvm/state/accounts.go Outdated Show resolved Hide resolved
fvm/state/accounts.go Outdated Show resolved Hide resolved
fvm/state/accounts.go Show resolved Hide resolved
@fxamacker
Copy link
Member

fxamacker commented Jul 15, 2022

@ramtinms after you mentioned slice vs array idea today, I looked into trade-off between type AccountStatus []byte and type AccountStatus [AccountStatusSize]byte.

The performance and allocations are similar if:

  • AccountStatusFromBytes(inp []byte) converts slice inp to array pointer without make a copy
  • AccountStatus functions use pointer receivers, such as func (a *AccountStatus) AccountExists() bool

The benefit of using array is to prevent creating AccoutStatus from byte slice of any length, such as

	b := []byte{1}
	acct := AccountStatus(b)

To be safer, I think we can use array as AccountStatus.

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks good to me (besides the things that Faye mentioned).

I am slightly concerned with the introduction of a MaxPublicKeyCount. Its definitely nice to have this control if we need it, but if we choose to use it we should be aware that keys cannot currently be removed, only revoked, which means that we need to know how we are going to help accounts that have Max-1 revoked keys, 1 good key and they want to switch that last key with a different one.

Base automatically changed from ramtin/fvm-remove-legacy-controller-from-storage to master July 15, 2022 23:56
@ramtinms ramtinms requested a review from AlexHentschel as a code owner July 15, 2022 23:56
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #2799 (e86d263) into master (34ef088) will decrease coverage by 0.08%.
The diff coverage is 63.71%.

@@            Coverage Diff             @@
##           master    #2799      +/-   ##
==========================================
- Coverage   56.32%   56.24%   -0.09%     
==========================================
  Files         684      690       +6     
  Lines       63047    63587     +540     
==========================================
+ Hits        35512    35762     +250     
- Misses      24520    24800     +280     
- Partials     3015     3025      +10     
Flag Coverage Δ
unittests 56.24% <63.71%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/bootstrap/cmd/block.go 50.00% <0.00%> (ø)
cmd/bootstrap/cmd/machine_account.go 63.09% <0.00%> (-3.16%) ⬇️
cmd/bootstrap/run/block.go 0.00% <0.00%> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/scaffold.go 13.89% <0.00%> (-0.06%) ⬇️
cmd/util/cmd/execution-state-extract/cmd.go 49.50% <0.00%> (+9.82%) ⬆️
...execution-state-extract/execution_state_extract.go 51.13% <0.00%> (ø)
cmd/util/ledger/migrations/accounts.go 0.00% <0.00%> (ø)
...d/util/ledger/migrations/storage_fees_migration.go 8.88% <0.00%> (+0.19%) ⬆️
cmd/util/ledger/reporters/atree_reporter.go 0.00% <ø> (ø)
... and 157 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01fae8e...e86d263. Read the comment docs.

@ramtinms
Copy link
Contributor Author

@janezpodhostnik I kept MaxPublicKeyCount and set it to maxUint64 to prevent overflowing for now and added to the TPM agenda if we want to have any lower value or concerns we have about it.

Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Nice work! Just a couple more minor comments.

Comment on lines +64 to +65
copy(as[:], inp)
return &as, nil
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can avoid making a copy by using input's underlying array. And we don't need as created earlier.

Suggested change
copy(as[:], inp)
return &as, nil
return (*AccountStatusArray)(inp), nil

Copy link
Contributor Author

@ramtinms ramtinms Jul 19, 2022

Choose a reason for hiding this comment

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

I was not sure, I was worried we might update the registers directly in this case and if we get rid of deep copy feature of ledger we might have some issues? what do you think ? or we could reuse but copy on updates

Copy link
Member

Choose a reason for hiding this comment

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

I was worried we might update the registers directly in this case

I think it's OK if AccountStatusFromBytes is only called to create account status by reading:

  • payload value from mtrie, or
  • cached payload value, previously retrieved from mtrie.

Because a copy of payload value is returned from mtrie, I think we update the copy.

if we get rid of deep copy feature of DB we might have some issues

Yes but getting rid of deep copy of the payload value can be problematic even without this change. I think it would be safer to get rid of deep copy of the payload key.

Copy link
Contributor Author

@ramtinms ramtinms Jul 19, 2022

Choose a reason for hiding this comment

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

Another case I'm still not sure, is the case we rollback, we drop the delta but the underlying read cache on view is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's keep this optimization for another PR when we revisit all allocations and possible ways to not copy or deep copy.

fvm/state/accounts_status.go Outdated Show resolved Hide resolved
@ramtinms ramtinms merged commit 1a221f1 into master Jul 19, 2022
@ramtinms ramtinms deleted the ramtin/fvm-merge-account-meta-registers branch July 19, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants