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

refactor(json): Restore gin support for app engine #1026

Merged
merged 5 commits into from
Jul 18, 2017

Conversation

appleboy
Copy link
Member

In order to restore Gin support for app engine (disable jsonite through tags), create new folder to support multiple JSON packages.

use jsoniter

$ go build -tags=jsoniter .

use default json

$ go build .

fix #1023

cc @javierprovecho @taowen @codedance

@codecov
Copy link

codecov bot commented Jul 17, 2017

Codecov Report

Merging #1026 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1026   +/-   ##
=======================================
  Coverage   96.61%   96.61%           
=======================================
  Files          16       16           
  Lines        1712     1712           
=======================================
  Hits         1654     1654           
  Misses         50       50           
  Partials        8        8
Impacted Files Coverage Δ
errors.go 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7180f2b...3e623c1. Read the comment docs.

@taowen
Copy link

taowen commented Jul 17, 2017

LGTM

this is a good example of go build tags

Copy link
Member

@javierprovecho javierprovecho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@appleboy can you add a section at README, explaining shortly how to compile/build using the jsonite tag? same as in the body of this PR 😉 thanks!

@appleboy
Copy link
Member Author

@javierprovecho Done. Already update readme.

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

I have objection to switch json engine to jsoniter since jsoniter doesn't have 100% compatibility to encoding/json.

package main

import (
	"fmt"
	"log"

	"github.com/json-iterator/go"
)

func test(s string) {
	var err error
	var v interface{}
	err = jsoniter.Unmarshal([]byte(s), &v)
	if err != nil {
		log.Print(err)
		return
	}
	fmt.Println(v)
}

func main() {
	test("--2")   // 2 [Why not error?]
	test("002")   // 2 [Why not error?]
	test("00.2")  // 0.2 [Why not error?]
	test("1e1")   // 10 [Why not float?]
	test("1.0e1") // 10 [Why not float?]
}

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn json-iterator/go#135 working on that

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn all fixed

@appleboy
Copy link
Member Author

@mattn @taowen See #1033 update vendor version.

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

Still not.

package main

import (
	"encoding/json"
	"log"
	"reflect"

	"github.com/json-iterator/go"
)

func test(s string) {
	var err1, err2 error
	var v1, v2 interface{}
	log.Println("testing:", s)
	err1 = jsoniter.Unmarshal([]byte(s), &v1)
	err2 = json.Unmarshal([]byte(s), &v2)
	if err1 != nil && err2 == nil {
		log.Printf("jsoniter return error but encoding/json not return error:", err1)
	} else if err1 == nil && err2 != nil {
		log.Printf("jsoniter not return error but encoding/json return error:", err2)
	} else if !reflect.DeepEqual(v1, v2) {
		log.Printf("there are different between jsoniter and encoding/json: %v, %v", v1, v2)
	}
}

func main() {
	test("-")
	test("--")
	test("0000000000000000000000000000000000000000000")
	test("1*1")
	test("1%")
	test("1&")
}
2017/07/18 19:10:28 testing: -
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: -0, <nil>
2017/07/18 19:10:28 testing: --
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: -0, <nil>
2017/07/18 19:10:28 testing: 0000000000000000000000000000000000000000000
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: 0, <nil>
2017/07/18 19:10:28 testing: 1*1
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: 1, <nil>
2017/07/18 19:10:28 testing: 1%
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: 1, <nil>
2017/07/18 19:10:28 testing: 1&
2017/07/18 19:10:28 there are different between jsoniter and encoding/json: 1, <nil>

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn have you updated the dependency? I can not reproduce the issues on latest master branch.

json/json.go Outdated
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

//+build !jsoniter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

json/jsoniter.go Outdated
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

//+build jsoniter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

@taowen Yes, head/master.

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn it has already returned error, I don not think the value difference is a big issue. User should check the error, and discard the value.

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

Maybe, difference still exists.

package main

import (
	"encoding/json"
	"log"
	"reflect"

	"github.com/json-iterator/go"
)

func test(s string) {
	var err1, err2 error
	var v1, v2 interface{}
	log.Printf("testing: %q", s)
	err1 = jsoniter.Unmarshal([]byte(s), &v1)
	err2 = json.Unmarshal([]byte(s), &v2)
	if err1 != nil && err2 == nil {
		log.Printf("jsoniter return error but encoding/json not return error: %v", err1)
	} else if err1 == nil && err2 != nil {
		log.Printf("jsoniter not return error but encoding/json return error: %v", err2)
	} else if err1 == nil && err2 == nil && !reflect.DeepEqual(v1, v2) {
		log.Printf("there are different between jsoniter and encoding/json: %v, %v", v1, v2)
	}
}

func main() {
	test("10.")
}

// 2017/07/18 20:54:40 testing: "10."
// 2017/07/18 20:54:40 jsoniter not return error but encoding/json return error: invalid character ' ' after decimal point in numeric literal

I will not use gin again unless it is a perfectly compatible with encoding/json.

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn "10." fixed. Surprise to find out strconv.ParseFloat think it is valid float.

Create new folder to support multiple json package.
restore gin support for app engine (disable jsonite through tags)

use jsoniter

$ go build -tags=jsoniter .

use default json

$ go build .

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

package main

import (
	"encoding/json"
	"log"
	"reflect"

	"github.com/json-iterator/go"
)

func test(s string) {
	var err1, err2 error
	var v1, v2 interface{}
	log.Printf("testing: %q", s)
	err1 = jsoniter.Unmarshal([]byte(s), &v1)
	err2 = json.Unmarshal([]byte(s), &v2)
	if err1 != nil && err2 == nil {
		log.Printf("jsoniter return error but encoding/json not return error: %v", err1)
	} else if err1 == nil && err2 != nil {
		log.Printf("jsoniter not return error but encoding/json return error: %v", err2)
	} else if err1 == nil && err2 == nil && !reflect.DeepEqual(v1, v2) {
		log.Printf("there are different between jsoniter and encoding/json: %v, %v", v1, v2)
	}
}

func main() {
	test(`"\uD83D"`)
	test(`"\U0001f64f"`)
}

// 2017/07/18 23:29:53 testing: "\"\\uD83D\""
// 2017/07/18 23:29:53 jsoniter return error but encoding/json not return error: ReadString: expects \u after utf16 surrogate, but \ not found, parsing 8 ..."\uD83D"... at "\uD83D"
// 2017/07/18 23:29:53 testing: "\"\\U0001f64f\""
// 2017/07/18 23:29:53 jsoniter not return error but encoding/json return error: invalid character 'U' in string escape code

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

Also

test(`"\uDEADBEEF"`)
2017/07/18 23:36:14 testing: "\"\\uDEADBEEF\""
2017/07/18 23:36:14 jsoniter return error but encoding/json not return error: ReadString: expects \u after utf16 surrogate, but \ not found, parsing 8 ..."\uDEADB... at "\uDEADBEEF"

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn surrogate issues, fixed

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

1.e1 still wrong.

2017/07/19 00:22:20 testing: "1.e1"
2017/07/19 00:22:20 jsoniter not return error but encoding/json return error: invalid character 'e' after decimal point in numeric literal

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn 1.e1 fixed

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

Benchmark_jsoniter_large_file-4           100000             23015 ns/op            5056 B/op          9 allocs/op
Benchmark_jsoniter_large_file-4           100000             23016 ns/op            5056 B/op          9 allocs/op
Benchmark_jsoniter_large_file-4           100000             23956 ns/op            5056 B/op          9 allocs/op
Benchmark_jsoniter_large_file-4           100000             23077 ns/op            5056 B/op          9 allocs/op
Benchmark_jsoniter_large_file-4           100000             23092 ns/op            5056 B/op          9 allocs/op
Benchmark_json_large_file-4               100000             22077 ns/op            1664 B/op         10 allocs/op
Benchmark_json_large_file-4               100000             22136 ns/op            1664 B/op         10 allocs/op
Benchmark_json_large_file-4               100000             22182 ns/op            1664 B/op         10 allocs/op
Benchmark_json_large_file-4               100000             22204 ns/op            1664 B/op         10 allocs/op
Benchmark_json_large_file-4               100000             24583 ns/op            1664 B/op         10 allocs/op

jsoniter is not faster than encoding/json.

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn please share your test code

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

I don't write code anything. Just test go test -bench large_file on json-iterator/go

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn Benchmark_xxx is not well maintained, I just updated that one json-iterator/go@12cd299

It was referencing to a non-existing file "/tmp/large-file.json"

The new implementation of Skip() does validation (old faster mode can be restored with --tags jsoniter-sloppy), but still faster than encoding/json . Here are results from my machine

Benchmark_jsoniter_large_file 200000	      8886 ns/op	    4336 B/op	       6 allocs/op
Benchmark_json_large_file      50000	     34244 ns/op	    6744 B/op	      14 allocs/op

@mattn
Copy link
Contributor

mattn commented Jul 18, 2017

I did point problem about JSON decoder. I'm not sure, but there are problems in encoder, probably. I don't have enough time to check them.

@taowen
Copy link

taowen commented Jul 18, 2017

@mattn thank you

@javierprovecho
Copy link
Member

@mattn this pr will make jsonite optional, so like nothing happened between v1.2 and future v1.3. I encourage everyone to use the vendoring system for avoiding unstable changes made between the latest version and next version.

@javierprovecho javierprovecho merged commit ce670a6 into gin-gonic:master Jul 18, 2017
SegFaultx64 added a commit to wearkinetic/gin that referenced this pull request Jul 18, 2017
@appleboy appleboy deleted the jsontag branch July 19, 2017 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore gin support for app engine (disable jsonite through tags)
5 participants