feat: add sonic json support #3184

Rainshaw · 2022-06-09T10:28:19Z

Introduce a new drop-in json enhancement: sonic.

Bisstocuz · 2022-06-22T02:50:54Z

Requirement

Go 1.15/1.16/1.17/1.18

Linux/MacOS/Windows

Amd64 CPU with AVX instruction set

Sonic DOSE NOT ensure to support all environments, due to the difficulty of developing high-performance codes. For developers who use sonic to build their applications in different environments (ex: developing on M1 Mac but running on linux server), or those who want to handle JSON strictly consistent with encoding/json, we provide some compatible APIs as sonic.API

It seems not completely drop-in.

Rainshaw · 2022-06-27T12:15:09Z

Requirement

Go 1.15/1.16/1.17/1.18

Linux/MacOS/Windows

Amd64 CPU with AVX instruction set

Sonic DOSE NOT ensure to support all environments, due to the difficulty of developing high-performance codes. For developers who use sonic to build their applications in different environments (ex: developing on M1 Mac but running on linux server), or those who want to handle JSON strictly consistent with encoding/json, we provide some compatible APIs as sonic.API

It seems not completely drop-in.

IMO we provide the ability and whether to use should depend on users. we can add more statement in readme.
now if user would like to use sonic, they must have read its docs and know its limitations, this should be supported by our framework

internal/json/sonic.go

thinkerou · 2022-06-28T10:06:34Z

@RainshawGao please update ci, see #3214

codecov · 2022-06-28T10:36:09Z

Codecov Report

Merging #3184 (97e7a5a) into master (79dd72d) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3184      +/-   ##
==========================================
- Coverage   98.37%   98.36%   -0.02%     
==========================================
  Files          43       42       -1     
  Lines        3148     3124      -24     
==========================================
- Hits         3097     3073      -24     
  Misses         38       38              
  Partials       13       13

Flag	Coverage Δ
go-1.15	`98.36% <ø> (-0.02%)`	⬇️
go-1.16	`?`
go-1.17	`98.27% <ø> (-0.02%)`	⬇️
go-1.18	`98.27% <ø> (-0.02%)`	⬇️
macos-latest	`98.36% <ø> (-0.02%)`	⬇️
nomsgpack	`?`
ubuntu-latest	`98.36% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
binding/binding_nomsgpack.go

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 79dd72d...97e7a5a. Read the comment docs.

thinkerou · 2022-06-28T10:40:35Z

unit test error on sonic tag.

Bisstocuz · 2022-06-29T08:14:18Z

It seems not completely drop-in.

IMO we provide the ability and whether to use should depend on users. we can add more statement in readme. now if user would like to use sonic, they must have read its docs and know its limitations, this should be supported by our framework

The better solution is for us to identify those incompatible scenarios and then provide compatibility code to ensure the ease of use of the Gin framework. We should add some build tag for available hardware architecture.

Rainshaw · 2022-06-29T11:05:16Z

It seems not completely drop-in.

IMO we provide the ability and whether to use should depend on users. we can add more statement in readme. now if user would like to use sonic, they must have read its docs and know its limitations, this should be supported by our framework

The better solution is for us to identify those incompatible scenarios and then provide compatibility code to ensure the ease of use of the Gin framework. We should add some build tag for available hardware architecture.

done, but the fail ci test should be considered more.... golang/go#25956 indicates whether one failed test in ci should be designed or modified still needs discussion.

Rainshaw · 2022-06-30T05:54:41Z

I dig into the fail test cases, all these cases are about trying to use newdecoder().decode on an empty string, sonic would not return EOF error while encoding/json does. Related issues golang/go#25956

Should this behavior must be the same?

Rainshaw · 2022-07-04T05:16:34Z

@Bisstocuz @thinkerou sonic has fixed the fail tests, see Rainshaw#1 with action https://github.com/RainshawGao/gin/actions/runs/2607644458
I have no idea with the error showed in the action summary page as https://github.com/gin-gonic/gin/actions/runs/2565859221 this action also have them.

internal/json/json.go

thinkerou · 2022-07-04T08:40:20Z

Makefile

 	echo "mode: count" > coverage.out
 	for d in $(TESTFOLDER); do \
-		$(GO) test -tags $(TESTTAGS) -v -covermode=count -coverprofile=profile.out $$d > tmp.out; \
+		$(GO) test $(TESTTAGS) -v -covermode=count -coverprofile=profile.out $$d > tmp.out; \


$(GO) test -tags "$(TESTTAGS)" -v ...
and test-tags is:
test-tags: ['', 'nomsgpack', 'sonic avx', 'go_json']
right?

oh, I try to fix the error showed in the action summary page, and move the -tags string to the action settings, but it seems not work..

test-tags: ['', '-tags nomsgpack', '-tags "sonic avx"', '-tags go_json']

should this be moved back?

thinkerou · 2022-07-04T14:14:43Z

waiting @appleboy approval, thanks!

Bisstocuz · 2022-07-05T03:07:19Z

Good job! But, I think avx tag is unneccessary, simply keeping sonic is perfect. We only need to ensure developers know about sonic's requirements, and make their Gin service stable if they mistakely use sonic.

appleboy · 2022-08-01T06:30:55Z

README.md

 ```sh
 go build -tags=go_json .
 ```
+[sonic](https://github.com/bytedance/sonic) (you have to ensure that your cpu support avx instruction.)


Add a blank line above.

thinkerou requested review from appleboy and thinkerou June 28, 2022 03:59

thinkerou added this to the v1.9 milestone Jun 28, 2022

thinkerou added the enhancement label Jun 28, 2022

thinkerou reviewed Jun 28, 2022

View reviewed changes

internal/json/sonic.go Outdated Show resolved Hide resolved

Rainshaw force-pushed the add_sonic branch from a9d1146 to 09e558d Compare June 28, 2022 09:28

Rainshaw mentioned this pull request Jun 30, 2022

One behavior diff with encoding/json bytedance/sonic#251

Closed

Rainshaw force-pushed the add_sonic branch 2 times, most recently from c7d9ee0 to 25caaee Compare July 4, 2022 04:16

thinkerou reviewed Jul 4, 2022

View reviewed changes

internal/json/json.go Outdated Show resolved Hide resolved

Rainshaw force-pushed the add_sonic branch from 25caaee to 2dae706 Compare July 4, 2022 07:13

thinkerou reviewed Jul 4, 2022

View reviewed changes

thinkerou previously approved these changes Jul 4, 2022

View reviewed changes

Rainshaw dismissed thinkerou’s stale review via ed6167c July 5, 2022 02:08

Rainshaw force-pushed the add_sonic branch from 2dae706 to ed6167c Compare July 5, 2022 02:08

thinkerou previously approved these changes Jul 6, 2022

View reviewed changes

feat: add sonic json support

b813da3

Rainshaw dismissed thinkerou’s stale review via b813da3 August 1, 2022 03:55

Rainshaw force-pushed the add_sonic branch from ed6167c to b813da3 Compare August 1, 2022 03:55

thinkerou previously approved these changes Aug 1, 2022

View reviewed changes

appleboy reviewed Aug 1, 2022

View reviewed changes

fix: add blank line in readme

97e7a5a

Rainshaw dismissed thinkerou’s stale review via 97e7a5a August 1, 2022 06:33

thinkerou approved these changes Aug 2, 2022

View reviewed changes

thinkerou merged commit 8374ed2 into gin-gonic:master Aug 2, 2022

alecholmez mentioned this pull request Jun 27, 2023

Why bytedance/sonic? #3653

Closed

feat: add sonic json support #3184

feat: add sonic json support #3184

Uh oh!

Conversation

Rainshaw commented Jun 9, 2022

Uh oh!

Bisstocuz commented Jun 22, 2022

Requirement

Uh oh!

Rainshaw commented Jun 27, 2022

Requirement

Uh oh!

Uh oh!

thinkerou commented Jun 28, 2022

Uh oh!

codecov bot commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thinkerou commented Jun 28, 2022

Uh oh!

Bisstocuz commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rainshaw commented Jun 29, 2022

Uh oh!

Rainshaw commented Jun 30, 2022

Uh oh!

Rainshaw commented Jul 4, 2022

Uh oh!

Uh oh!

thinkerou Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rainshaw Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

thinkerou commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bisstocuz commented Jul 5, 2022

Uh oh!

appleboy Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rainshaw Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 28, 2022 •

edited

Loading

Bisstocuz commented Jun 29, 2022 •

edited

Loading

thinkerou Jul 4, 2022 •

edited

Loading

thinkerou commented Jul 4, 2022 •

edited

Loading

appleboy Aug 1, 2022 •

edited

Loading