Skip to content
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

encoding/json: stateEndTop allocates an error it doesn't return #17914

Closed
sandymcp opened this issue Nov 14, 2016 · 8 comments
Closed

encoding/json: stateEndTop allocates an error it doesn't return #17914

sandymcp opened this issue Nov 14, 2016 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Milestone

Comments

@sandymcp
Copy link

sandymcp commented Nov 14, 2016

What version of Go are you using (go version)?

go version go1.7 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/almcpherson/gopath"
GORACE=""
GOROOT="/Users/almcpherson/go1.7"
GOTOOLDIR="/Users/almcpherson/go1.7/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/jv/2c5lqc993lgdnm85hggybmw43959v8/T/go-build941373586=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Running benchmarks to see why
func stateEndTop(s *scanner, c byte) int

is making calls to

func (s *scanner) error(c byte, context string) int

on valid json.

package mapping

import (
	"encoding/json"
	"testing"
)

func BenchmarkLittleJSON(b *testing.B) {
	little := []byte(`{ "hello": "world", "x": {"n": 1, "m":2}, "world": ["1","2","3"], "what": "heh?"}`)
	for i := 0; i < b.N; i++ {
		var result struct {
			Hello string `json:"hello"`
			World []string `json:"world"`
		}
		if err := json.Unmarshal(little, &result); err != nil {
			b.Error(err)
		}
	}
}

https://play.golang.org/p/Fsvp3vGwXO

go test -bench "BenchmarkLittleJSON" -cpuprofile c.out
go tool pprof -peek error test.test c.out

What did you expect to see?

no calls to error

What did you see instead?

calls to error

1.18s of 1.18s total ( 100%)
----------------------------------------------------------+-------------
flat flat% sum% cum cum% calls calls% + context
----------------------------------------------------------+-------------
0.21s 100% | encoding/json.stateEndTop
0 0% 0% 0.21s 17.80% | encoding/json.(*scanner).error
0.11s 52.38% | runtime.concatstring4
0.07s 33.33% | encoding/json.quoteChar
0.03s 14.29% | runtime.newobject
----------------------------------------------------------+-------------

@tejasmanohar
Copy link

May be worth providing a runnable example

@sandymcp
Copy link
Author

Actually according to the comment for stateEndTop it seems it is intended to be called only when the top-level closes. However it is called multiple times. I put a print statement in stateEndTop. On the simple example above it gets called twice: once with ' ' and once with '}' as the argument c.

// stateEndTop is the state after finishing the top-level value,
// such as after reading `{}` or `[1,2,3]`.
// Only space characters should be seen now.
func stateEndTop(s *scanner, c byte) int {

@sandymcp sandymcp reopened this Nov 14, 2016
@sandymcp sandymcp changed the title Broken check in json/scanner.go json/scanner.go stateEndTop being called too frequently and causing allocation of errors to occur Nov 14, 2016
@sandymcp
Copy link
Author

At the end of an object or array in the first level of the json object the parseLevel falls back to 0 and the code calls stateEndTop.

@quentinmit
Copy link
Contributor

I don't understand the bug here; it looks to me like the Unmarshal call is correctly returning a nil error. It sounds like you think there is some optimization to be done?

@quentinmit quentinmit changed the title json/scanner.go stateEndTop being called too frequently and causing allocation of errors to occur encoding/json: stateEndTop allocates an error it doesn't return Nov 15, 2016
@quentinmit quentinmit added NeedsFix The path to resolution is known, but the work has not been done. Performance labels Nov 15, 2016
@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 15, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33276 mentions this issue.

gopherbot pushed a commit that referenced this issue Jun 29, 2017
This reverts commit df68afd (https://golang.org/cl/33276)

Reason for revert: made other benchmarks worse

Fixes #20693 (details)
Updates #17914
Updates #10335

Change-Id: If451b620803ccb0536b89c76c4353d2185d57d7e
Reviewed-on: https://go-review.googlesource.com/47211
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/47152 mentions this issue: encoding/json: read ahead after value consumption

@gopherbot
Copy link

Change https://golang.org/cl/47211 mentions this issue: Revert "encoding/json: reduce unmarshal mallocs for unmapped fields"

@bradfitz
Copy link
Contributor

Assigned to @rsc for review of https://go-review.googlesource.com/c/go/+/47152

@golang golang locked and limited conversation to collaborators Mar 1, 2019
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants