[Execution] Do not depend on string conversion in register set tracking #3529
Conversation
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
Codecov Report
@@ Coverage Diff @@
## master #3529 +/- ##
==========================================
+ Coverage 55.22% 59.90% +4.68%
==========================================
Files 755 149 -606
Lines 69164 16961 -52203
==========================================
- Hits 38196 10161 -28035
+ Misses 27834 5972 -21862
+ Partials 3134 828 -2306
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
SaveTheRbtz
force-pushed
the
rbtz/optimizeLedger
branch
from
a6cd1b8
to
e181b4c
Compare
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 6f64410 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SaveTheRbtz
changed the title
SaveTheRbtz
force-pushed
the
rbtz/optimizeLedger
branch
from
8bbe910
to
fcd604d
Compare
6 tasks
SaveTheRbtz
force-pushed
the
rbtz/optimizeLedger
branch
2 times, most recently
from
c29ccb7
to
3b6de45
Compare
ramtinms
reviewed
Nov 8, 2022
Comment on lines
20
to
+21
| func (r *RegisterID) String() string { | ||
| ownerLen := len(r.Owner) | ||
|
|
||
| requiredLen := ((ownerLen + len(r.Key)) * 2) + 1 | ||
|
|
||
| arr := make([]byte, requiredLen) | ||
|
|
||
| hex.Encode(arr, []byte(r.Owner)) | ||
|
|
||
| arr[2*ownerLen] = byte('/') | ||
|
|
||
| hex.Encode(arr[(2*ownerLen)+1:], []byte(r.Key)) | ||
|
|
||
| return string(arr) | ||
| return fmt.Sprintf("%x/%x", r.Owner, r.Key) |
There was a problem hiding this comment.
I'm not sure why we were doing it this way in the first place, I think it suppose to be some sort of optimized way of doing it
There was a problem hiding this comment.
yeah. It is about 2x fast as the Sprintf. The main problem here is that it was not needed at all -- we were using this for map indexing, instead of just using the struct itself (which is infinitely faster and does not require allocations.)
SaveTheRbtz
requested review from
janezpodhostnik,
pattyshack,
m4ksio and
psiemens
pattyshack
approved these changes
Nov 8, 2022
|
|
||
| for _, v := range d.Data { | ||
| data = append(data, v) | ||
| for k, v := range d.Data { |
There was a problem hiding this comment.
This seems like a waste. sorting then splitting into two list. maybe just return the original sorted list, or change the sorter
There was a problem hiding this comment.
I was going to suggest a similar thing, I think we don't need to sort based on both key and values since the key is used for the Data map is going to be anyway unique, sorting based on the key but appending the values while we are doing it through the sort method might be the more optimized approach here. I see with the current code you were trying to maintain the same logic but I think in this case is fine to change the sort to be based on the key, considering the less implementation of the KeyEntry is only limited to keys.
SaveTheRbtz
force-pushed
the
rbtz/optimizeLedger
branch
from
3b6de45
to
a1b0c64
Compare
ramtinms
approved these changes
Nov 8, 2022
|
Ok. I'll land this as is to minimize the updates and followup with the PR to simplify |
This PR:
map[string]flow.RegisterIDtomap[flow.RegisterID]struct{}to represent sets.map[string]flow.RegisterEntrytomap[flow.RegisterID]flow.RegisterValueto save a bit of space.Currently these are responsible for ~15% of memory allocations:
