-
Notifications
You must be signed in to change notification settings - Fork 489
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
performance: Don't preallocate rarely used maps in MakeStateDelta #4715
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
7ad0465
Delay calling make on stateDeltas for rare fields
iansuvak b2747a3
actually remove Creatables
iansuvak 199a6b3
add test
iansuvak 2ff1349
add comments
iansuvak 2a11528
Merge remote-tracking branch 'upstream/master' into make_state_delta_gcc
iansuvak c66fab0
Merge remote-tracking branch 'upstream/master' into make_state_delta_gcc
cce 33ec03b
Exclude KvMods and address review feedback
iansuvak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,9 +96,11 @@ type StateDelta struct { | |
Txids map[transactions.Txid]IncludedTransactions | ||
|
||
// new txleases for the txtail mapped to expiration | ||
// not pre-allocated so use .UpsertTxleases to modify instead of direct assignment | ||
Txleases map[Txlease]basics.Round | ||
|
||
// new creatables creator lookup table | ||
// not pre-allocated so use .UpsertCreatables to modify instead of direct assignment | ||
Creatables map[basics.CreatableIndex]ModifiedCreatable | ||
|
||
// new block header; read-only | ||
|
@@ -180,9 +182,11 @@ type AccountDeltas struct { | |
// AppResources deltas. If app params or local state is deleted, there is a nil value in AppResources.Params or AppResources.State and Deleted flag set | ||
AppResources []AppResourceRecord | ||
// caches for {addr, app id} to app params delta resolution | ||
// not preallocated - use UpsertAppResource instead of inserting directly | ||
appResourcesCache map[AccountApp]int | ||
|
||
AssetResources []AssetResourceRecord | ||
AssetResources []AssetResourceRecord | ||
// not preallocated - use UpsertAssertResource instead of inserting directly | ||
assetResourcesCache map[AccountAsset]int | ||
} | ||
|
||
|
@@ -191,12 +195,10 @@ type AccountDeltas struct { | |
// This does not play well for AssetConfig and ApplicationCall transactions on scale | ||
func MakeStateDelta(hdr *bookkeeping.BlockHeader, prevTimestamp int64, hint int, stateProofNext basics.Round) StateDelta { | ||
return StateDelta{ | ||
Accts: MakeAccountDeltas(hint), | ||
KvMods: make(map[string]KvValueDelta), | ||
Txids: make(map[transactions.Txid]IncludedTransactions, hint), | ||
Txleases: make(map[Txlease]basics.Round), | ||
Accts: MakeAccountDeltas(hint), | ||
KvMods: make(map[string]KvValueDelta), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. KvMods seems like a candidate for the same treatment. The vast majority of txns will not have any KvMods. |
||
Txids: make(map[transactions.Txid]IncludedTransactions, hint), | ||
// asset or application creation are considered as rare events so do not pre-allocate space for them | ||
Creatables: make(map[basics.CreatableIndex]ModifiedCreatable), | ||
Hdr: hdr, | ||
StateProofNext: stateProofNext, | ||
PrevTimestamp: prevTimestamp, | ||
|
@@ -209,9 +211,6 @@ func MakeAccountDeltas(hint int) AccountDeltas { | |
return AccountDeltas{ | ||
Accts: make([]BalanceRecord, 0, hint*2), | ||
acctsCache: make(map[basics.Address]int, hint*2), | ||
|
||
appResourcesCache: make(map[AccountApp]int), | ||
assetResourcesCache: make(map[AccountAsset]int), | ||
} | ||
} | ||
|
||
|
@@ -403,6 +402,22 @@ func (ad *AccountDeltas) UpsertAssetResource(addr basics.Address, aidx basics.As | |
ad.assetResourcesCache[key] = last | ||
} | ||
|
||
// UpsertTxLease adds a new TxLease to the StateDelta | ||
func (sd *StateDelta) UpsertTxLease(txLease Txlease, expired basics.Round) { | ||
iansuvak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if sd.Txleases == nil { | ||
sd.Txleases = make(map[Txlease]basics.Round) | ||
} | ||
sd.Txleases[txLease] = expired | ||
} | ||
|
||
// UpsertCreatable adds a new Creatable to the StateDelta | ||
func (sd *StateDelta) UpsertCreatable(idx basics.CreatableIndex, creatable ModifiedCreatable) { | ||
if sd.Creatables == nil { | ||
sd.Creatables = make(map[basics.CreatableIndex]ModifiedCreatable) | ||
} | ||
sd.Creatables[idx] = creatable | ||
} | ||
|
||
// OptimizeAllocatedMemory by reallocating maps to needed capacity | ||
// For each data structure, reallocate if it would save us at least 50MB aggregate | ||
// If provided maxBalLookback or maxTxnLife are zero, dependent optimizations will not occur. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
could write a quick test and see if the new code has more allocations than the old one? - wondering if compiler optimizes it right.
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.
Did you mean for this function in particular? or in general?
I ran the
BenchmarkMakeStateDelta
that already existed. Out of the box it's the same on master and this branch at 5allocs/op but when adding//go:noinline
toMakeStateDelta
the difference becomes 7 vs 12 as expected.That seems odd to me since running
go build --gcflags="-m" .
inledger/ledgercore
gives this showing that the allocs are escaping to heap when compiled and scenario1 run above confirms it: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.
then maybe
AddCreatable
should take a pointer to get rid of additional copying?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.
It looks like this is already merged but do you want me to add it? This should be called rarely and the objects are tiny so the copy cost is not big but should be safe