restore: reduce allocations when restoring to longer tenant keys #120149
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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 |
| @@ -273,7 +274,19 @@ func (kr *KeyRewriter) RewriteKey( | |||
| copy(keyTenantPrefix, newTenantPrefix) | |||
| rekeyed = append(keyTenantPrefix, rekeyed...) | |||
| } else { | |||
| rekeyed = append(newTenantPrefix, rekeyed...) | |||
| l := len(newTenantPrefix) + len(rekeyed) | |||
There was a problem hiding this comment.
could you write a comment explaining why you're doing this? I believe the idea is that you want to reduce the number of times we allocate memory by allocating a slab of memory every time we exhaust our alloc buf.
There was a problem hiding this comment.
it also seems that we can use this strategy here too, though I think this path in traditional restore.
There was a problem hiding this comment.
that we can use this strategy here too
We could. we'd need to hang the slab off the rule instead of the rewriter or something. I'm not very motivated until I see it in a profile, which would probably be from backing up table < 127 and restoring > 127.
Eh, I think microbenchmarking is useful if we're really hunting for optimizations, but otherwise I'm fine with just letting a profile from a running real cluster tell us when something is worth optimizing. I don't feel particularly motivated to add KeyRewriter benchmarks especially as OR replaces this code. |
Release note: none. Epic: none.
|
TFTR! bors r+ |
|
Build succeeded: |
Release note: none.
Epic: none.