base: optimize {NodeIDContainer,StoreIDContainer}.String() #70539
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)
pkg/util/tracing/span_inner.go, line 85 at r1 (raw file):
// This span is likely only used to carry payloads around. // // TODO(andrei): The optimization for avoiding the tags was done back when
Do y'all have an opinion here?
pkg/base/node_id.go
Outdated
| } | ||
|
|
||
| // SafeFormat implements the redact.SafeFormatter interface. | ||
| func (s *StoreIDContainer) SafeFormat(w redact.SafePrinter, _ rune) { | ||
| val := s.Get() | ||
| if val == 0 { |
There was a problem hiding this comment.
I'd move all of this logic into Set or you can have races where val != 0 but str is still nil.
andreimatei
force-pushed
the
tracing.NodeIdContainer-string
branch
from
382b536
to
4ad93b3
Compare
These String() methods were implemented in terms of their respective SafeFormat, which was pretty expensive: upwards of 750ns and between 4-7 allocations depending on the node id. This cost caused at least two workarounds, that the patch annotates. The patch makes stringifying cheap by precomputing the value and moving from SafeFormatter to SafeValue. Moving away from SafeFormatter to a more down-to-earth implementation brings the cost down to between 0 and 1 allocations. But I went further and precomputed the value because these containers are used as logging tags and so can easily end up being stringified very frequently. Release note: None
andreimatei
force-pushed
the
tracing.NodeIdContainer-string
branch
from
4ad93b3
to
295689b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/base/node_id.go, line 108 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
I'd move all of this logic into
Setor you can have races whereval != 0butstris still nil.
You're right about the race. I've fixed it by only looking at str in this method.
I've moved the "temp" logic to Set, since it belongs better there.
But beyond that, moving the"?" logic would mean that I need a constructor for StoreIDContainer and that the zero value is no good. I don't really want that.
Good?
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build succeeded: |
These String() methods were implemented in terms of their respective
SafeFormat, which was pretty expensive: upwards of 750ns and between 4-7
allocations depending on the node id. This cost caused at least two
workarounds, that the patch annotates.
The patch makes stringifying cheap by precomputing the value and moving
from SafeFormatter to SafeValue. Moving away from SafeFormatter to a
more down-to-earth implementation brings the cost down to between 0 and
1 allocations. But I went further and precomputed the value because
these containers are used as logging tags and so can easily end up
being stringified very frequently.
Release note: None