Proposal Details
The jsontext.Encoder
and jsontext.Decoder
APIs both include a Reset(..., opts ...Options)
method for reconfiguring it "such that it is [reading/writing] afresh from [r/w] and configured with the provided options." At present these are both implemented in such a way that calling them forces a rather large allocation. I'd like to propose an optimization to improve the reuse potential of both types by avoiding that allocation when possible.
My use case involves pooling short-lived Encoder
and Decoder
values with sync.Pool
and reusing them via their Reset
method. Two important notes specific to my usage:
pprof
showed that that the forced allocations account for about 50% of total allocations (alloc_space
) in my use case.- I never actually need to change the options—I configure them once up front and then call
enc.Reset(newW, enc.Options())
on each reuse.
I think it's possible and worthwhile for both Encoder.Reset
and Decoder.Reset
to be allocation-free in this scenario.
cc @dsnet
Possible solution
The source of the allocation is the handling the provided opts
. Specifically, each call to Reset
currently allocates a new 120-byte jsonopts.Struct
struct and merges the new opts
into it.
Of the approaches I considered, the simplest is to recognize when the given opts
are exactly identical to the existing jsonopts.Struct
and forego the needless reconstruction:
diff --git jsontext/encode.go jsontext/encode.go
index 997820f..d123354 100644
--- jsontext/encode.go
+++ jsontext/encode.go
@@ -114,9 +114,11 @@ func (e *encoderState) reset(b []byte, w io.Writer, opts ...Options) {
if bb, ok := w.(*bytes.Buffer); ok && bb != nil {
e.Buf = bb.Bytes()[bb.Len():] // alias the unused buffer of bb
}
+ if unchanged := len(opts) == 1 && opts[0] == &e.Struct; !unchanged {
opts2 := jsonopts.Struct{} // avoid mutating e.Struct in case it is part of opts
opts2.Join(opts...)
e.Struct = opts2
+ }
if e.Flags.Get(jsonflags.Multiline) {
if !e.Flags.Has(jsonflags.SpaceAfterColon) {
e.Flags.Set(jsonflags.SpaceAfterColon | 1)
This seems safe to me but I'm interested in the assessment of someone familiar with the inner workings of jsontext
and jsonopts
.
I verified via pprof
that this change indeed eliminates any allocations if the caller uses the enc.Reset(newW, enc.Options())
pattern. If accepted, it may additionally be worth considering documenting Reset
to be allocation-free if used in this way.
Comment From: dsnet
Hi, thanks for the proposal, but I don't believe this needs to be a proposal, since we should optimize Reset to reduce allocations, regardless. I'll switch this to a normal bug report.
There are some recent and outstanding optimizations that might actually already fix this: * [CL/685135] encoding/json/v2: avoid escaping jsonopts.Struct * [CL/681177] encoding/json/jsontext: preserve buffer capacity in Decoder.Reset
Comment From: tylerchr
Ok, thanks for the guidance. I wondered.
Great! Admittedly I'm actually using github.com/go-json-experiment/json
—are you planning to continue merging changes back to there? If not, once they land here I can re-test with a tip compiler.
Comment From: dsnet
I just merged https://github.com/go-json-experiment/json/pull/178, which should pull in the optimization.
Comment From: tylerchr
It works! Preventing join
from escaping its receiver seems to be the change that does it. Thanks for going out of your way to backport that so promptly.
I looked at the CL out of curiosity. For my own education: I see why escaping the receiver in join
would heap-allocate opts2
in reset
, but I don't know if I quite understand why join
escaped its receiver in the first place. Your commit message says it's
because it is passed to JoinUnknownOption, which is a dynamically implemented function
I would think that the compiler can tell that jsonopts.JoinUnknownOption
, at least, doesn't escape its argument. Am I understanding correctly that because JoinUnknownOption
is an exported dynamic function that can be (and is) overridden from another package at runtime, the compiler can't know when compiling jsonopts
that the runtime implementation of JoinUnknownOption
doesn't escape it and therefore has to assume that it might? And that assumption then percolates back up through Join
into reset
where opts2
gets needlessly heap-allocated?
Comment From: dsnet
Yep, you are correct in your assessment.
A smarter compiler might be able to figure out when compiling an entire program that there is practically only one implementation of JoinUnknownOption
and analyze that single implementation for escape analysis. However, this analysis is impossible to do when compiling just the encoding/json/internal/jsonopts
package in isolation, which has no knowledge of external circumstances.
Comment From: prattmic
It sounds like this is fixed with https://go.dev/cl/685135 and https://go.dev/cl/681177?
I'm going to optimistically close this. Please let me know if I got it wrong.