-
Notifications
You must be signed in to change notification settings - Fork 123
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
[tstate] change map[key] from string to fixed size byte array #170
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Could you change the The string conversion I was doing definitely was not optimal...this is probably the right approach. |
You should make a similar change here: Lines 44 to 79 in a61157d
|
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
Signed-off-by: Sam Batschelet <[email protected]>
3b773d1
to
d8abb92
Compare
Signed-off-by: Sam Batschelet <[email protected]>
for _, k := range tx.StateKeys(sm) { | ||
sk := string(k) | ||
sk := tstate.ToStateKeyArray(k) |
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.
I wonder if the best middle-ground is just to use the approach in BenchmarkOptimizedStringKeyed
?
https://pthevenet.com/posts/programming/go/bytesliceindexedmaps/
We then get most of the benefit while retaining flexibility 🤷 .
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.
yeah the flexibility loss isn't worth the gain, working on benchmark a few alternatives including yours, hope to have that soon.
This PR has become stale because it has been open for 30 days with no activity. Adding the |
This is an experimentation with replacing the current internal maps from
map[string][]byte
tomap[[65]byte][]byte
. In benchmarks I found that using a byte array was much faster than using string as a key. I chose the length based on the largest keys I saw used by transfervm for warp. As you can see by the profiles this cut out a fairly expensive overhead ofmappassign_faststr
.ref. https://gitlab.com/pthevenet/go-small-answers/-/blob/29c03407006a269f8f6ba27fa3123fb9742ddf7c/byte-slice-indexed-maps/benchmark_test.go
This reduces the allocations for
FetchAndSetScope
and provides much cleaner profile (will upload files soon).before

after
