sql: more allocation reductions #57208
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/conn_executor_exec.go, line 281 at r7 (raw file):
queryDone := func(ctx context.Context, res RestrictedCommandResult) { if timeoutTicker != nil { timeoutTicker.Stop()
[nit] I'd point out that if Stop fails, the callback might still run or is in the process of running (and explain why that's ok)
pkg/sql/conn_executor_exec.go, line 1030 at r13 (raw file):
var ( eventStartImplicitTxn = fsm.Event(eventTxnStart{ImplicitTxn: fsm.True})
[nit] usually this is written as var eventStartImplicitTxn fsm.Event = eventTxnStart{..}
pkg/sql/opt/memo/interner.go, line 403 at r14 (raw file):
func (h *hasher) HashType(val *types.T) { // NOTE: we don't construct a perfect hash of the type, because it's
[nit] can simplify this and just say that we hash the type's OID and a few important fields, and that IsTypeEqual makes the final call about what types are identical.
jordanlewis
force-pushed
the
cancelcheckers
branch
from
83b700c to
b545275
Compare
Release note: None
Remove a closure for query unregistration as well make the allocation of a channel lazy. Release note: None
Release note: None
`connExecutor` used 2 allocating closures on every transaction for callbacks that are executed on transaction end. This is unnecessary, and this commit replaces them with methods on the `connExecutor`. Release note: None
Previously, we unconditionally constructed a new string in a log method when doing it in a format string under V() was sufficient. Release note: None
Previously, we used Type.String() as the hash function to intern types. Constructing a type's string can be very expensive, though, because of tuple and array types: these require allocating new strings. Tuples are particularly bad: they require allocating O(log n) in the number of elements in the tuple! Now, we construct a hash function that operates on cheaper fields of the type. Note that it doesn't have to be completely precise, just as before, since we always re-check type equality later. Release note: None
Previously, we'd construct a new conn fsm event, allocating each time due to the interface conversion, on every transaction start. Now, we use a static one. Events are immutable, so this is safe. Release note: None
Previously, it was a separate heap allocated object. Release note: None
yuzefovich
force-pushed
the
cancelcheckers
branch
from
b545275 to
0027621
Compare
There was a problem hiding this comment.
I rebased this PR, modified some commits a bit, and addressed all the comments. @RaduBerinde PTAL.
I also dropped the commit about rowenc: cheapen AdjustKeyForInterleave since we're removing the interleaved code anyway. Let me know if you'd rather see it included in the PR.
Reviewed 9 of 23 files at r24, 2 of 2 files at r26, 1 of 2 files at r27, 1 of 2 files at r28, 1 of 2 files at r29, 1 of 3 files at r30, 3 of 3 files at r31.
Reviewable status:
pkg/sql/conn_executor_exec.go, line 281 at r7 (raw file):
Previously, RaduBerinde wrote…
[nit] I'd point out that if Stop fails, the callback might still run or is in the process of running (and explain why that's ok)
I've thought a bit about this, and I'm not sure why that would be ok, so I reverted the change partially.
pkg/sql/conn_executor_exec.go, line 1030 at r13 (raw file):
Previously, RaduBerinde wrote…
[nit] usually this is written as
var eventStartImplicitTxn fsm.Event = eventTxnStart{..}
Done.
pkg/sql/opt/memo/interner.go, line 403 at r14 (raw file):
Previously, RaduBerinde wrote…
[nit] can simplify this and just say that we hash the type's OID and a few important fields, and that
IsTypeEqualmakes the final call about what types are identical.
Done.
|
@cucaroach tagging you as an additional reviewer here. |
|
Friendly ping @RaduBerinde @cucaroach |
There was a problem hiding this comment.
Reviewed 23 of 23 files at r24, 1 of 1 files at r25, 2 of 2 files at r26, 2 of 2 files at r27, 2 of 2 files at r28.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
See individual commits for details.