kv/rangefeed: reduce size of event struct from 200 bytes to 72 bytes #88308
Conversation
|
This change is |
There was a problem hiding this comment.
Nice, thanks!
Reviewed 2 of 2 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/rangefeed/processor.go line 164 at r1 (raw file):
type ctEvent struct { hlc.Timestamp
nit: why not type ctEvent hlc.Timestamp?
pkg/kv/kvserver/rangefeed/processor.go line 177 at r1 (raw file):
type syncEvent struct { c chan struct{} // This setting is used in conjunction with syncC in tests in order to ensure
nit: update to refer to c rather than syncC.
This commit restructures the event struct and reduces its size from 200 bytes to 72 bytes. This is accomplished primarily by pushing large, infrequently used struct fields into pointers. This is mostly just a drive-by cleanup. Release justification: None. Don't merge yet. Release note: None.
Slight style nit that makes the code less noisy.
nvanbenschoten
force-pushed
the
nvanbenschoten/rangefeedEventSize
branch
from
2b126b9
to
11ab1ec
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/kv/kvserver/rangefeed/processor.go line 164 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
nit: why not
type ctEvent hlc.Timestamp?
That's what I had initially, but type ctEvent hlc.Timestamp does not inherit methods while embedding the timestamp does, so this made some code less verbose.
For instance, this allows:
case !e.ct.IsEmpty():
p.forwardClosedTS(ctx, e.ct.Timestamp)instead of:
case !hlc.Timestamp(e.ct).IsEmpty():
p.forwardClosedTS(ctx, hlc.Timestamp(e.ct))Happy to change it if you feel strongly though.
pkg/kv/kvserver/rangefeed/processor.go line 177 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
nit: update to refer to
crather thansyncC.
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status:
pkg/kv/kvserver/rangefeed/processor.go line 164 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
That's what I had initially, but
type ctEvent hlc.Timestampdoes not inherit methods while embedding the timestamp does, so this made some code less verbose.For instance, this allows:
case !e.ct.IsEmpty(): p.forwardClosedTS(ctx, e.ct.Timestamp)instead of:
case !hlc.Timestamp(e.ct).IsEmpty(): p.forwardClosedTS(ctx, hlc.Timestamp(e.ct))Happy to change it if you feel strongly though.
Not at all, this is fine.
nvanbenschoten
changed the title
|
bors r+ |
|
Build succeeded: |
This commit restructures the event struct and reduces its size from 200 bytes to 72 bytes. This is accomplished primarily by pushing large, infrequently used struct fields into pointers. This is mostly just a drive-by cleanup found while working on #77724.
Release justification: None. Don't merge yet.
Release note: None.