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

Comments

Projects
None yet
6 participants
@sandymcp
Copy link

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

This comment has been minimized.

Copy link

commented Nov 14, 2016

May be worth providing a runnable example

@sandymcp

This comment has been minimized.

Copy link
Author

commented Nov 14, 2016

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 closed this Nov 14, 2016

@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

This comment has been minimized.

Copy link
Author

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2016

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 this to the Go1.9Maybe milestone Nov 15, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Nov 16, 2016

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

@gopherbot gopherbot closed this in df68afd Mar 20, 2017

gopherbot pushed a commit that referenced this issue Jun 29, 2017

Revert "encoding/json: reduce unmarshal mallocs for unmapped fields"
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

This comment has been minimized.

Copy link

commented Jul 27, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Jul 27, 2017

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

@bradfitz bradfitz reopened this Feb 19, 2018

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.11 Feb 19, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

@gopherbot gopherbot closed this in 5d11838 Mar 1, 2018

@golang golang locked and limited conversation to collaborators Mar 1, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.