kv: avoid extra key allocations in optimizePuts #104669
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Nice catch! Do you have benchmark results that demonstrate the impact of this change?
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @tbg)
pkg/kv/kvserver/replica_evaluate.go line 58 at r1 (raw file):
// Returns false on occurrence of a duplicate key. maybeAddPut := func(key roachpb.Key) bool { // The lookup will not copy key but the put will, but since map doesn't
s/the put/the map insertion/
pkg/kv/kvserver/replica_evaluate.go line 61 at r1 (raw file):
// escape and we don't mutate the keys its safe to not // allocate for both. mapKey := *(*string)(unsafe.Pointer(&key))
Want to call encoding.UnsafeConvertBytesToString here?
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_evaluate.go line 58 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
s/the put/the map insertion/
Done
pkg/kv/kvserver/replica_evaluate.go line 61 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Want to call
encoding.UnsafeConvertBytesToStringhere?
At first I didn't want to introduce a new dependency but now I see other files in this package already depend on util/encoding so +1.
I tried a couple things but failed to extract any meaningful signal, this cost is just a drop in the bucket it seems. But its good to fix and maybe some day when the rest of the bulk insert path is better optimized changes like this will become more important. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r=nvanbenschoten |
|
Build failed: |
The code correctly pointed out that the lookup didn't allocate a copy, the go compiler intelligently constructs a string on the stack pointing directly to the byte slice. However if the string isn't in the map it has to allocate a new string for the put. Now we avoid that. Release note: none Epic: none
|
bors r=nvanbenschoten |
|
Build succeeded: |
The code correctly pointed out that the lookup didn't allocate a copy,
the go compiler intelligently constructs a string on the stack pointing
directly to the byte slice. However if the string isn't in the map it
has to allocate a new string for the put. Now we avoid that.
Release note: none
Epic: none