Skip to content

feat: add sonic json support#3184

Merged
thinkerou merged 2 commits intogin-gonic:masterfrom
Rainshaw:add_sonic
Aug 2, 2022
Merged

feat: add sonic json support#3184
thinkerou merged 2 commits intogin-gonic:masterfrom
Rainshaw:add_sonic

Conversation

@Rainshaw
Copy link
Copy Markdown
Contributor

@Rainshaw Rainshaw commented Jun 9, 2022

Introduce a new drop-in json enhancement: sonic.

@Bisstocuz
Copy link
Copy Markdown
Contributor

Bisstocuz commented Jun 22, 2022

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
Copy link
Copy Markdown
Contributor Author

Rainshaw commented Jun 27, 2022

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

@thinkerou thinkerou requested review from appleboy and thinkerou June 28, 2022 03:59
@thinkerou thinkerou added this to the v1.9 milestone Jun 28, 2022
thinkerou
thinkerou reviewed Jun 28, 2022
View reviewed changes
Comment thread internal/json/sonic.go Outdated
@thinkerou
Copy link
Copy Markdown
Member

thinkerou commented Jun 28, 2022

@RainshawGao please update ci, see #3214

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 28, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Member

thinkerou commented Jun 28, 2022

unit test error on sonic tag.

@Bisstocuz
Copy link
Copy Markdown
Contributor

Bisstocuz commented Jun 29, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

Rainshaw commented Jun 29, 2022

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 Rainshaw mentioned this pull request Jun 30, 2022
Closed
@Rainshaw
Copy link
Copy Markdown
Contributor Author

Rainshaw commented Jun 30, 2022

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 Rainshaw force-pushed the add_sonic branch 2 times, most recently from c7d9ee0 to 25caaee Compare July 4, 2022 04:16
@Rainshaw
Copy link
Copy Markdown
Contributor Author

Rainshaw commented Jul 4, 2022

@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.

thinkerou
thinkerou reviewed Jul 4, 2022
View reviewed changes
Comment thread internal/json/json.go Outdated
thinkerou
thinkerou reviewed Jul 4, 2022
View reviewed changes
Comment thread 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; \
Copy link
Copy Markdown
Member

@thinkerou thinkerou Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@Rainshaw Rainshaw Jul 4, 2022

Choose a reason for hiding this comment

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

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
thinkerou previously approved these changes Jul 4, 2022
View reviewed changes
@thinkerou
Copy link
Copy Markdown
Member

thinkerou commented Jul 4, 2022 •
edited
Loading

waiting @appleboy approval, thanks!

@Bisstocuz
Copy link
Copy Markdown
Contributor

Bisstocuz commented Jul 5, 2022

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.

thinkerou
thinkerou previously approved these changes Jul 6, 2022
View reviewed changes
thinkerou
thinkerou previously approved these changes Aug 1, 2022
View reviewed changes
appleboy
appleboy reviewed Aug 1, 2022
View reviewed changes
Comment thread README.md
```sh
go build -tags=go_json .
```
[sonic](https://github.com/bytedance/sonic) (you have to ensure that your cpu support avx instruction.)
Copy link
Copy Markdown
Member

@appleboy appleboy Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Add a blank line above.

Copy link
Copy Markdown
Contributor Author

@Rainshaw Rainshaw Aug 1, 2022

Choose a reason for hiding this comment

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

done

@thinkerou thinkerou merged commit 8374ed2 into gin-gonic:master Aug 2, 2022
@alecholmez alecholmez mentioned this pull request Jun 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@appleboy appleboy appleboy left review comments

@thinkerou thinkerou thinkerou approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

v1.9

Development

Successfully merging this pull request may close these issues.

4 participants

@Rainshaw @Bisstocuz @thinkerou @appleboy