sql: don't copy maps in prepStmtNamespace.resetTo if identical
#134183
Conversation
This commit optimizes prepStmtNamespace.resetTo for the common case under commitPrepStmtNamespace where the prepStmtNamespace maps are identical. In these cases, deleting and re-inserting into the hash maps can cause hash map resizing and incur heap allocations. Longer term, we should reconsider the relationship between `prepStmtsNamespace` and `prepStmtsNamespaceAtTxnRewindPos`, ideally using a scheme that allows for a constant-time "reset" operation in the common case. This could be accomplished using a copy-on-write scheme or by maintaining some extra state about how the two have diverged since the last reset. For now, we get 90% of the way there by testing for equality before copying maps. ``` name old time/op new time/op delta Sysbench/SQL/oltp_read_only 36.2ms ± 3% 36.7ms ± 3% ~ (p=0.095 n=10+9) Sysbench/SQL/oltp_write_only 19.0ms ± 2% 19.1ms ± 1% ~ (p=0.297 n=9+9) Sysbench/SQL/oltp_read_write 60.1ms ± 6% 58.2ms ± 4% -3.15% (p=0.004 n=9+10) Sysbench/SQL/oltp_point_select 1.61ms ± 2% 1.57ms ± 2% -2.24% (p=0.001 n=9+9) Sysbench/SQL/oltp_begin_commit 587µs ± 2% 543µs ± 2% -7.58% (p=0.000 n=9+9) name old alloc/op new alloc/op delta Sysbench/SQL/oltp_read_only 1.05MB ± 0% 1.05MB ± 0% ~ (p=0.730 n=9+9) Sysbench/SQL/oltp_write_only 403kB ± 1% 398kB ± 1% -1.17% (p=0.001 n=10+9) Sysbench/SQL/oltp_read_write 1.51MB ± 1% 1.49MB ± 0% -1.20% (p=0.000 n=10+8) Sysbench/SQL/oltp_point_select 35.0kB ± 0% 31.7kB ± 0% -9.28% (p=0.000 n=10+10) Sysbench/SQL/oltp_begin_commit 18.8kB ± 0% 14.8kB ± 0% -21.29% (p=0.000 n=9+9) name old allocs/op new allocs/op delta Sysbench/SQL/oltp_read_only 6.94k ± 0% 6.95k ± 1% ~ (p=0.348 n=9+9) Sysbench/SQL/oltp_write_only 3.62k ± 1% 3.61k ± 0% ~ (p=0.067 n=10+9) Sysbench/SQL/oltp_read_write 11.0k ± 1% 10.9k ± 1% -0.91% (p=0.000 n=9+9) Sysbench/SQL/oltp_point_select 317 ± 0% 309 ± 0% -2.37% (p=0.000 n=10+10) Sysbench/SQL/oltp_begin_commit 139 ± 0% 130 ± 0% -6.74% (p=0.000 n=10+10) ``` Epic: None Release note: None
![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 |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/sql/conn_executor.go line 2061 at r1 (raw file):
for name, ps := range ns.prepStmts { ps.decRef(ctx) delete(ns.prepStmts, name)
[nit] Maybe doing a loop for decRef, then a clear, then a maps.Copy, and then a loop for incRef would be a bit faster?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/sql/conn_executor.go line 2061 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] Maybe doing a loop for
decRef, then aclear, then amaps.Copy, and then a loop forincRefwould be a bit faster?
maps.Copy isn't doing anything special, so I don't think this will be meaningfully faster. I'll leave as is for now, but we can revisit if we want to optimize this further.
|
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Sure, my thinking was that clear is faster than deleting each element. Note that a simple loop that just deletes elements is recognized by the compiler (so it's probably equivalent to clear), but probably not when the loop is more complicated. |
This commit optimizes prepStmtNamespace.resetTo for the common case under commitPrepStmtNamespace where the prepStmtNamespace maps are identical. In these cases, deleting and re-inserting into the hash maps can cause hash map resizing and incur heap allocations.
Longer term, we should reconsider the relationship between
prepStmtsNamespaceandprepStmtsNamespaceAtTxnRewindPos, ideally using a scheme that allows for a constant-time "reset" operation in the common case. This could be accomplished using a copy-on-write scheme or by maintaining some extra state about how the two have diverged since the last reset. For now, we get 90% of the way there by testing for equality before copying maps.Informs: #32427.
Epic: None
Release note: None