Skip to content

encoding/json: don't reset before returning buffer to pool #34195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

andig
Copy link
Contributor

@andig andig commented Sep 9, 2019

Reset is already performed when retrieving from pool

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Sep 9, 2019
@andig andig changed the title encoding/json: Don't reset when putting to pool encoding/json: don't reset before returning buffer to pool Sep 9, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: d9df74a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/194338 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Andreas Goetz:

Patch Set 1:

Kindly asking for review as you've done on c054747


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=62f068c3


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

R=mvdan

This seems consistent with the other usage of the pool in stream.go.


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 1:

Decent benchmark numbers from this change in encoding/json:

name old time/op new time/op delta
CodeEncoder-8 2.48ms ± 9% 2.07ms ± 4% -16.36% (p=0.008 n=5+5)
CodeMarshal-8 2.58ms ± 1% 2.25ms ± 3% -12.88% (p=0.008 n=5+5)
MarshalBytes/32-8 265ns ± 2% 255ns ± 1% -3.70% (p=0.008 n=5+5)
MarshalBytes/256-8 637ns ± 2% 624ns ± 2% ~ (p=0.071 n=5+5)
MarshalBytes/4096-8 6.23µs ±12% 5.64µs ± 1% -9.47% (p=0.008 n=5+5)
CodeDecoder-8 11.0ms ± 3% 10.2ms ± 1% -7.21% (p=0.008 n=5+5)
UnicodeDecoder-8 259ns ± 0% 247ns ± 1% -4.63% (p=0.008 n=5+5)
DecoderStream-8 210ns ± 1% 206ns ± 1% -1.81% (p=0.016 n=5+5)
CodeUnmarshal-8 11.4ms ± 2% 11.2ms ± 4% ~ (p=0.222 n=5+5)
CodeUnmarshalReuse-8 10.3ms ± 1% 10.1ms ± 1% -2.30% (p=0.016 n=4+5)
UnmarshalString-8 120ns ± 2% 122ns ±10% ~ (p=0.913 n=5+5)
UnmarshalFloat64-8 112ns ± 8% 110ns ± 5% ~ (p=0.452 n=5+5)
UnmarshalInt64-8 90.5ns ± 2% 90.3ns ± 1% ~ (p=1.000 n=5+5)
Issue10335-8 159ns ± 3% 156ns ± 2% ~ (p=0.119 n=5+5)
Unmapped-8 357ns ± 5% 365ns ± 6% ~ (p=0.762 n=5+5)
TypeFieldsCache/MissTypes1-8 9.55µs ± 1% 9.27µs ± 7% ~ (p=0.151 n=5+5)
TypeFieldsCache/MissTypes10-8 35.6µs ± 8% 36.3µs ± 9% ~ (p=0.690 n=5+5)
TypeFieldsCache/MissTypes100-8 190µs ± 3% 184µs ± 9% ~ (p=0.310 n=5+5)
TypeFieldsCache/MissTypes1000-8 1.67ms ± 3% 1.58ms ± 6% -5.78% (p=0.032 n=5+5)
TypeFieldsCache/MissTypes10000-8 15.8ms ± 2% 17.2ms ± 9% +8.71% (p=0.008 n=5+5)
TypeFieldsCache/MissTypes100000-8 167ms ± 4% 162ms ± 7% ~ (p=0.548 n=5+5)
TypeFieldsCache/MissTypes1000000-8 1.69s ±26% 1.63s ±27% ~ (p=0.421 n=5+5)
TypeFieldsCache/HitTypes1-8 10.1ns ± 6% 8.8ns ± 2% -12.84% (p=0.008 n=5+5)
TypeFieldsCache/HitTypes10-8 9.10ns ± 2% 8.49ns ± 3% -6.72% (p=0.008 n=5+5)
TypeFieldsCache/HitTypes100-8 9.51ns ± 4% 9.26ns ± 2% ~ (p=0.095 n=5+5)
TypeFieldsCache/HitTypes1000-8 9.06ns ± 6% 8.96ns ± 3% ~ (p=1.000 n=5+5)
TypeFieldsCache/HitTypes10000-8 9.02ns ± 4% 8.36ns ± 6% -7.29% (p=0.032 n=5+5)
TypeFieldsCache/HitTypes100000-8 9.29ns ± 4% 9.52ns ± 6% ~ (p=0.151 n=5+5)
TypeFieldsCache/HitTypes1000000-8 10.7ns ± 7% 9.5ns ± 9% ~ (p=0.095 n=5+5)
NumberIsValid-8 17.3ns ± 6% 16.8ns ± 5% ~ (p=0.095 n=5+5)
NumberIsValidRegexp-8 384ns ± 3% 379ns ± 1% ~ (p=0.452 n=5+5)
EncoderEncode-8 70.4ns ± 2% 68.1ns ± 3% ~ (p=0.056 n=5+5)

name old speed new speed delta
CodeEncoder-8 784MB/s ± 9% 936MB/s ± 4% +19.28% (p=0.008 n=5+5)
CodeMarshal-8 752MB/s ± 1% 863MB/s ± 3% +14.82% (p=0.008 n=5+5)
CodeDecoder-8 177MB/s ± 3% 191MB/s ± 1% +7.71% (p=0.008 n=5+5)
UnicodeDecoder-8 54.0MB/s ± 1% 56.6MB/s ± 1% +4.81% (p=0.008 n=5+5)
CodeUnmarshal-8 170MB/s ± 2% 173MB/s ± 4% ~ (p=0.222 n=5+5)
CodeUnmarshalReuse-8 188MB/s ± 1% 192MB/s ± 1% +2.35% (p=0.016 n=4+5)

name old alloc/op new alloc/op delta
CodeEncoder-8 5.80B ±66% 4.60B ±57% ~ (p=0.238 n=5+5)
CodeMarshal-8 1.94MB ± 0% 1.96MB ± 3% ~ (p=0.190 n=4+5)
CodeDecoder-8 2.71MB ± 3% 2.64MB ± 1% -2.42% (p=0.008 n=5+5)
UnicodeDecoder-8 36.0B ± 0% 36.0B ± 0% ~ (all equal)
DecoderStream-8 8.00B ± 0% 8.00B ± 0% ~ (all equal)
CodeUnmarshal-8 3.28MB ± 0% 3.28MB ± 0% +0.00% (p=0.032 n=5+5)
CodeUnmarshalReuse-8 1.93MB ± 0% 1.93MB ± 0% ~ (p=0.056 n=5+5)
UnmarshalString-8 192B ± 0% 192B ± 0% ~ (all equal)
UnmarshalFloat64-8 180B ± 0% 180B ± 0% ~ (all equal)
UnmarshalInt64-8 176B ± 0% 176B ± 0% ~ (all equal)
Issue10335-8 200B ± 0% 200B ± 0% ~ (all equal)
Unmapped-8 232B ± 0% 232B ± 0% ~ (all equal)
EncoderEncode-8 0.00B 0.00B ~ (all equal)

name old allocs/op new allocs/op delta
CodeEncoder-8 0.00 0.00 ~ (all equal)
CodeMarshal-8 1.00 ± 0% 1.00 ± 0% ~ (all equal)
CodeDecoder-8 77.5k ± 0% 77.4k ± 0% -0.12% (p=0.008 n=5+5)
UnicodeDecoder-8 2.00 ± 0% 2.00 ± 0% ~ (all equal)
DecoderStream-8 1.00 ± 0% 1.00 ± 0% ~ (all equal)
CodeUnmarshal-8 92.7k ± 0% 92.7k ± 0% ~ (all equal)
CodeUnmarshalReuse-8 77.4k ± 0% 77.4k ± 0% ~ (p=0.063 n=5+5)
UnmarshalString-8 2.00 ± 0% 2.00 ± 0% ~ (all equal)
UnmarshalFloat64-8 2.00 ± 0% 2.00 ± 0% ~ (all equal)
UnmarshalInt64-8 1.00 ± 0% 1.00 ± 0% ~ (all equal)
Issue10335-8 3.00 ± 0% 3.00 ± 0% ~ (all equal)
Unmapped-8 4.00 ± 0% 4.00 ± 0% ~ (all equal)
EncoderEncode-8 0.00 0.00 ~ (all equal)


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 1:

Do you think this is better than CL 183228? If not, let's focus on that CL instead.


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Andreas Goetz:

Patch Set 1:

Patch Set 1:

Do you think this is better than CL 183228? If not, let's focus on that CL instead.

It's a three line problem, so not much possible difference. I think this CL is "better" in that its puts the reset logic in a single place while in CL 183228 its scattered around pool consumers. This one has 1 line less code.

Whatever CL implements this change the form of this one is slightly cleaner imho.


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Sep 10, 2019
Reset is already performed when retrieving from pool

Change-Id: Ia810dd18d3e55a1565a5ad435a00d1e46724576c
GitHub-Last-Rev: d9df74a
GitHub-Pull-Request: #34195
Reviewed-on: https://go-review.googlesource.com/c/go/+/194338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 1: Code-Review+2

I agree this is slightly better. And like Brad says, more consistent. LGTM


Please don’t reply on this GitHub thread. Visit golang.org/cl/194338.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/194338 has been merged.

@gopherbot gopherbot closed this Sep 10, 2019
Dirbaio pushed a commit to sqlbunny/json that referenced this pull request Sep 12, 2019
Reset is already performed when retrieving from pool

Change-Id: Ia810dd18d3e55a1565a5ad435a00d1e46724576c
GitHub-Last-Rev: d9df74a4aeb86e5d292c9fc33568a3c9a64a967d
GitHub-Pull-Request: golang/go#34195
Reviewed-on: https://go-review.googlesource.com/c/go/+/194338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
Reset is already performed when retrieving from pool

Change-Id: Ia810dd18d3e55a1565a5ad435a00d1e46724576c
GitHub-Last-Rev: d9df74a4aeb86e5d292c9fc33568a3c9a64a967d
GitHub-Pull-Request: golang/go#34195
Reviewed-on: https://go-review.googlesource.com/c/go/+/194338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
Reset is already performed when retrieving from pool

Change-Id: Ia810dd18d3e55a1565a5ad435a00d1e46724576c
GitHub-Last-Rev: d9df74a4aeb86e5d292c9fc33568a3c9a64a967d
GitHub-Pull-Request: golang/go#34195
Reviewed-on: https://go-review.googlesource.com/c/go/+/194338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
Reset is already performed when retrieving from pool

Change-Id: Ia810dd18d3e55a1565a5ad435a00d1e46724576c
GitHub-Last-Rev: d9df74a4aeb86e5d292c9fc33568a3c9a64a967d
GitHub-Pull-Request: golang/go#34195
Reviewed-on: https://go-review.googlesource.com/c/go/+/194338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants