performance: Don't preallocate rarely used maps in MakeStateDelta #4715
Conversation
ledger/internal/assetcow.go
Outdated
| cs.mods.Creatables[basics.CreatableIndex(index)] = ledgercore.ModifiedCreatable{ | ||
| Ctype: basics.AssetCreatable, | ||
| Creator: addr, | ||
| Created: false, | ||
| } | ||
| cs.mods.UpsertCreatable( | ||
| basics.CreatableIndex(index), | ||
| ledgercore.ModifiedCreatable{ | ||
| Ctype: basics.AssetCreatable, | ||
| Creator: addr, | ||
| Created: false, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
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.
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 to MakeStateDelta the difference becomes 7 vs 12 as expected.
That seems odd to me since running go build --gcflags="-m" . in ledger/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.
then maybe AddCreatable should take a pointer to get rid of additional copying?
There was a problem hiding this comment.
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
ledger/ledgercore/statedelta.go
Outdated
| 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.
KvMods seems like a candidate for the same treatment. The vast majority of txns will not have any KvMods.
Codecov Report
@@ Coverage Diff @@
## master #4715 +/- ##
=======================================
Coverage 54.62% 54.62%
=======================================
Files 414 414
Lines 53517 53533 +16
=======================================
+ Hits 29231 29245 +14
+ Misses 21864 21862 -2
- Partials 2422 2426 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| for key, value := range cb.mods.KvMods { | ||
| cb.commitParent.mods.KvMods[key] = value | ||
| cb.commitParent.mods.AddKvMod(key, value) | ||
| } |
There was a problem hiding this comment.
I guess it might have been slightly more performant to do:
if len(cb.mods.KvMods) {
cb.commitParent.mods.KvMods = make(map[string]KvValueDelta)
}
for key, value := range cb.mods.KvMods {
cb.commitParent.mods.KvMods[key] = value
}
|
How about making a MergeKvMods method that takes a map and puts the whole
thing in, only needing to check size and allocate once. And hey I guess
allocate with a size hunt while you're at it.
…On Fri, Nov 4, 2022, 4:35 PM cce ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ledger/internal/cow.go
<#4715 (comment)>
:
> for key, value := range cb.mods.KvMods {
- cb.commitParent.mods.KvMods[key] = value
+ cb.commitParent.mods.AddKvMod(key, value)
}
I guess it might have been slightly more performant to do:
if len(cb.mods.Kvmods) {
cb.commitParent.mods.KvMods = make(map[string]KvValueDelta)
}
for key, value := range cb.mods.KvMods {
cb.commitParent.mods.KvMods[key] = value
}
—
Reply to this email directly, view it on GitHub
<#4715 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7TYAYRJEJ6DV6LIFCZDWGVXSZANCNFSM6AAAAAARTOKEDQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Summary
This PR removes explicit definitions for Txlease and Creatables maps in MakeStateDelta and make{app|resource}Caches in MakeAccountDelta.
The latter already had upsert methods defined that would allocate an empty map if it was nil. This creates matching methods for the remaining two fields and propagates them wherever they are used.
This is a risky change because some code could be added in the future that tries to insert into the nil-map. Open to suggestions or closing this PR if the risk is not worth it
Test Plan
Existing tests pass and added new tests for the new methods.
On scenario1s pprof this reduces alloc_objects metric as compared to master as percentage of all objects allocated: