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

Add build tag nomsgpack #1852

Merged
merged 5 commits into from
Jan 7, 2020
Merged

Conversation

sapk
Copy link
Contributor

@sapk sapk commented Apr 8, 2019

This PR add a build tag nomsgpack to exclude heavy deps github.com/ugorji/go/codec that may be not needed for most project. The goal is to reduce the resulting binary size. This could maybe improve some performances but that not the goal so I haven't bench it.

I go for the no impact by default by using a negate tag so that without change it will not impact project depending on this lib.

I will try to provide some results of binary size based on https://github.com/gin-gonic/examples. -> see comment

Should fix (or at least a mitigation to) #1847

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #1852 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1852      +/-   ##
=========================================
+ Coverage   98.49%   98.5%   +0.01%     
=========================================
  Files          40      41       +1     
  Lines        2256    2276      +20     
=========================================
+ Hits         2222    2242      +20     
  Misses         18      18              
  Partials       16      16
Impacted Files Coverage Δ
binding/msgpack.go 100% <ø> (ø) ⬆️
render/msgpack.go 100% <ø> (ø) ⬆️
render/render.go 100% <ø> (ø) ⬆️
binding/binding.go 100% <ø> (ø) ⬆️
binding/binding_nomsgpack.go 100% <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 b8a7b6d...090d654. Read the comment docs.

@sapk
Copy link
Contributor Author

sapk commented Apr 8, 2019

Using the basic example :

go build ./
go build -ldflags="-s -w" -o basic.stripped ./
go build -tags nomsgpack -o basic.nomsgpack ./
go build -tags nomsgpack -ldflags="-s -w" -o basic.nomsgpack.stripped ./
ls -l basic*
-rwxr-xr-x 1 sapk sapk 17129377  9 avril 01:17 basic
-rwxr-xr-x 1 sapk sapk 12749851  9 avril 01:18 basic.nomsgpack
-rwxr-xr-x 1 sapk sapk  9342944  9 avril 01:21 basic.nomsgpack.stripped
-rwxr-xr-x 1 sapk sapk 12755840  9 avril 01:22 basic.stripped

Without the nomsgpack flag the resulting binary file are 35% bigger.

I used go version go1.12.1 linux/amd64 with in go.mod replace github.com/gin-gonic/gin => github.com/sapk-fork/gin v1.3.1-0.20190408225948-e3c972a151c8

@zzjin
Copy link
Contributor

zzjin commented Dec 24, 2019

sounds good for ci. any update here?

@appleboy
Copy link
Member

@sapk Could you resolve the conflicts?

@appleboy appleboy added this to the 1.6 milestone Dec 26, 2019
@sapk
Copy link
Contributor Author

sapk commented Dec 27, 2019

@lunny done

@lunny
Copy link

lunny commented Dec 28, 2019

@sapk :)

appleboy
appleboy previously approved these changes Dec 28, 2019
@appleboy
Copy link
Member

@thinkerou what do you think about this PR?

@sapk
Copy link
Contributor Author

sapk commented Dec 28, 2019

To re-validate, I re-did the size bench after last rebase.

-rwxr-xr-x 1 sapk sapk 15269181 28 déc.  19:41 basic
-rwxr-xr-x 1 sapk sapk 12925564 28 déc.  19:40 basic.nomsgpack
-rwxr-xr-x 1 sapk sapk  9506816 28 déc.  19:41 basic.nomsgpack.stripped
-rwxr-xr-x 1 sapk sapk 11239424 28 déc.  19:40 basic.stripped

@@ -0,0 +1,111 @@
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

  1. please update copyright
  2. filename: binding_nomsgpack.go binding_msgpack_test.go, should we use xxx_msgpack_xxx?
  3. the file and binding.go have repeat code, should we use reduce?
    thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Copyright 2020 Gin Core Team. All rights reserved.

I will have fresh look on removing/refactor duplicate code later.

@appleboy appleboy merged commit fd8a65b into gin-gonic:master Jan 7, 2020
@appleboy
Copy link
Member

appleboy commented Jan 7, 2020

maybe need another PR to update documentation.

@sapk sapk deleted the gin-no-msgpack branch January 7, 2020 10:10
@sapk
Copy link
Contributor Author

sapk commented Jan 7, 2020

@lunny for the doc you mean in the README.md ?

@appleboy
Copy link
Member

appleboy commented Jan 7, 2020

@sapk like as https://github.com/gin-gonic/gin#build-with-jsoniter

ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
* add build tag nomsgpack

* Update copyright

* Update copyright
byebyebruce pushed a commit to byebyebruce/gin that referenced this pull request Mar 25, 2020
* add build tag nomsgpack

* Update copyright

* Update copyright
@lowang-bh
Copy link

I hear that msgpack‘s performance is better than json.

@duaneking
Copy link

I'm loving this change and I'm glad it was made because I just did the math and calculated that it will save a company actual money.

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.

7 participants