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

Provide an int64 version of Quantity that is much faster #25243

Merged
merged 4 commits into from
May 19, 2016

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented May 6, 2016

Resolves #25019 by adding an int64 version of Quantity that is aware of overflow and can spill to the inf.Dec version. Heavily optimized all the paths for allocation and efficiency.

BenchmarkQuantityString-8           10000000           175 ns/op          23 B/op          2 allocs/op
BenchmarkQuantityStringBinarySI-8   10000000           214 ns/op          32 B/op          2 allocs/op
BenchmarkQuantityMarshalJSON-8      10000000           156 ns/op          35 B/op          1 allocs/op
BenchmarkQuantityUnmarshalJSON-8    10000000           194 ns/op           2 B/op          1 allocs/op
BenchmarkParseQuantity-8            10000000           142 ns/op           0 B/op          0 allocs/op
BenchmarkCanonicalize-8             20000000            89.2 ns/op         3 B/op          0 allocs/op
BenchmarkQuantityCopy-8             20000000            55.0 ns/op        48 B/op          1 allocs/op
BenchmarkQuantityAdd-8              50000000            33.0 ns/op         0 B/op          0 allocs/op
BenchmarkQuantityRound-8            50000000            25.1 ns/op         0 B/op          0 allocs/op
BenchmarkScaledValueSmall-8         30000000            35.9 ns/op         0 B/op          0 allocs/op
BenchmarkScaledValueLarge-8          3000000           511 ns/op          48 B/op          1 allocs/op

Known issues

  • Does not respect the current 12E max value, since that can be represented as 12*10^18 without loss of precision
  • Shifting the scale down to merge the denominator with numerator in Parse (which avoids going to inf.Dec), i.e. "1.2" becomes "12e-1". Questionable.
  • Implement protobuf as a string.

This change is Reviewable

@smarterclayton smarterclayton changed the title Explore optimizations of Quantity Experiment: Explore optimizations of Quantity May 6, 2016
@smarterclayton smarterclayton changed the title Experiment: Explore optimizations of Quantity DO NOT MERGE - Explore optimizations of Quantity May 6, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels May 6, 2016
@bgrant0607 bgrant0607 assigned timstclair and unassigned bgrant0607 May 6, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2016
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 9, 2016
@smarterclayton smarterclayton force-pushed the explore_quantity branch 5 times, most recently from ab12e56 to 7e6a27e Compare May 9, 2016 22:48
@smarterclayton smarterclayton changed the title DO NOT MERGE - Explore optimizations of Quantity Provide an int64 version of Quantity that is much faster May 9, 2016
@smarterclayton smarterclayton added the sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. label May 9, 2016
@smarterclayton smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 10, 2016
@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@smarterclayton
Copy link
Contributor Author

Naw, not going to squash.

@smarterclayton
Copy link
Contributor Author

Spawned #25886 for reenabling powerpc

@smarterclayton smarterclayton added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 19, 2016
@smarterclayton
Copy link
Contributor Author

Bumping since this has the powerpc disablement and blocks proto.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 19, 2016

GCE e2e build/test passed for commit 2c9b83f.

@lavalamp
Copy link
Member

Manual merge.

@lavalamp lavalamp merged commit 5448400 into kubernetes:master May 19, 2016
@bpradipt
Copy link

@luxas, first of all let me take this opportunity to sincerely thank you for the work you did to add the PowerPC arch support. Kubernetes on PowerPC is very important for IBM and we are ramping up the effort as well. Looking forward to your continued support and guidance. We are looking into the linker issue and will keep the community posted.

@wojtek-t
Copy link
Member

Ref #8132

@timstclair
Copy link

timstclair commented May 20, 2016

I just wanted to publicly acknowledge the horror that is circular vendor dependencies... I recognize there was no way to make this change without manually modifying the vendored appc package, but this now adds a bunch of etxra steps to updating Godeps. Can we move the resource package to its own repo?

@smarterclayton
Copy link
Contributor Author

We can.

On Fri, May 20, 2016 at 2:41 PM, Tim St. Clair notifications@github.com
wrote:

I just wanted to publicly acknowledge the horror that is circular vendor
dependencies... I recognize there was no way to make this change without
manually modifying the vendored appc package, but this now adds a bunch of
etxra steps to updating Godeps. Can we move the resource package to its own
repo?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25243 (comment)

@laboger
Copy link

laboger commented May 25, 2016

I'm looking into the PPC64 linker error. As mentioned above, the problem occurs when building the hyperkube binary. The intermediate go.o file that is generated contains a single text segment that is too large (>2**24 bytes).

An interesting question was brought up above that relates to this problem. This file seems to be pulling in many, many packages. Has anyone investigated whether or not they are all really needed? Unfortunately you are hitting this error on ppc64le but it indicates that even when the programs are successfully built maybe they are larger than they need to be. Also, as new code gets developed the programs will continue to grow in size and you will probably at some point hit linker overflow errors like this on other platforms.

@smarterclayton
Copy link
Contributor Author

Kube is probably one of the largest Go ecosystem projects (juju is larger,
and openshift because it depends on Kube).

Honestly, I expect us to grow to at least 2x our current package set size
over the next year - there will certainly be some time when we try to
reduce dependencies, but I don't see that happening soon and we'd prefer to
just fix the problems in the compile chains.

On Wed, May 25, 2016 at 10:23 AM, Lynn Boger notifications@github.com
wrote:

I'm looking into the PPC64 linker error. As mentioned above, the problem
occurs when building the hyperkube binary. The intermediate go.o file that
is generated contains a single text segment that is too large (>2**24
bytes).

An interesting question was brought up above that relates to this problem.
This file seems to be pulling in many, many packages. Has anyone
investigated whether or not they are all really needed? Unfortunately you
are hitting this error on ppc64le but it indicates that even when the
programs are successfully built maybe they are larger than they need to be.
Also, as new code gets developed the programs will continue to grow in size
and you will probably at some point hit linker overflow errors like this on
other platforms.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25243 (comment)

@laboger
Copy link

laboger commented May 25, 2016

The plan is to fix the problem so that the GNU linker can handle the build of a golang program like this. I was just wondering if the size of the program was a concern and if there were other ways to reduce the size.

@smarterclayton
Copy link
Contributor Author

It's not a concern per se - some folks have even made statements like
"we'll get to 1Gb binaries eventually" :). I doubt we'll get that bad, but
200Mb Kube binaries seems possible.

On Wed, May 25, 2016 at 12:48 PM, Lynn Boger notifications@github.com
wrote:

The plan is to fix the problem so that the GNU linker can handle the build
of a golang program like this. I was just wondering if the size of the
program was a concern and if there were other ways to reduce the size.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25243 (comment)

@thockin
Copy link
Member

thockin commented May 27, 2016

Hey. This is wrong. This is totally and utterly wrong. It makes our vendored code not reproducible. I can no longer godep restore/save and get my original values back. This has to roll back.

@smarterclayton
Copy link
Contributor Author

I believe upstream has fixed their circular dependency on us now, so we can
update and still compile.

On Fri, May 27, 2016 at 6:22 PM, Tim Hockin notifications@github.com
wrote:

Hey. This is wrong. This is totally and utterly wrong. It makes our
vendored code not reproducible. I can no longer godep restore/save and get
my original values back. This has to roll back.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25243 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pwatZAbUEIZbYCZXCpbfDZAsBOP_ks5qF26MgaJpZM4IYjvP
.

@smarterclayton
Copy link
Contributor Author

Correction - updated their circular dependency.

On Fri, May 27, 2016 at 6:24 PM, Clayton Coleman ccoleman@redhat.com
wrote:

I believe upstream has fixed their circular dependency on us now, so we
can update and still compile.

On Fri, May 27, 2016 at 6:22 PM, Tim Hockin notifications@github.com
wrote:

Hey. This is wrong. This is totally and utterly wrong. It makes our
vendored code not reproducible. I can no longer godep restore/save and get
my original values back. This has to roll back.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25243 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pwatZAbUEIZbYCZXCpbfDZAsBOP_ks5qF26MgaJpZM4IYjvP
.

@smarterclayton
Copy link
Contributor Author

appc 0.8.4 has the change.

On Fri, May 27, 2016 at 6:25 PM, Clayton Coleman ccoleman@redhat.com
wrote:

Correction - updated their circular dependency.

On Fri, May 27, 2016 at 6:24 PM, Clayton Coleman ccoleman@redhat.com
wrote:

I believe upstream has fixed their circular dependency on us now, so we
can update and still compile.

On Fri, May 27, 2016 at 6:22 PM, Tim Hockin notifications@github.com
wrote:

Hey. This is wrong. This is totally and utterly wrong. It makes our
vendored code not reproducible. I can no longer godep restore/save and get
my original values back. This has to roll back.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25243 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pwatZAbUEIZbYCZXCpbfDZAsBOP_ks5qF26MgaJpZM4IYjvP
.

@thockin
Copy link
Member

thockin commented May 27, 2016

no, upstream for github.com/appc/spec/schema/types/isolator_resources.go is not fixed - ParseQuantity returns a value now, and they need a pointer

@smarterclayton
Copy link
Contributor Author

ab50d12e88f57788bf84b83fef2be236eb1fcc0b has it. The latter reverted?

On Fri, May 27, 2016 at 6:29 PM, Tim Hockin notifications@github.com
wrote:

no, upstream for github.com/appc/spec/schema/types/isolator_resources.go
is not fixed - ParseQuantity returns a value now, and they need a pointer


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25243 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_va1vdR3JDatNJBavrJJDNbOAtvks5qF3A8gaJpZM4IYjvP
.

@thockin
Copy link
Member

thockin commented May 27, 2016

7da0fdd4dfce6fda81869ac6ecd6ef42247957ea undid it

@smarterclayton
Copy link
Contributor Author

I'm going to bump to #26459
ab50d12e88 until they pick up our next tag.

On Fri, May 27, 2016 at 6:42 PM, Tim Hockin notifications@github.com
wrote:

7da0fdd4dfce6fda81869ac6ecd6ef42247957ea undid it


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25243 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_LICO7GgOotyVAq9_a16MFylha6ks5qF3NmgaJpZM4IYjvP
.

@smarterclayton
Copy link
Contributor Author

Opened #26463 to verify that verify-godeps.sh is doing its job - pretty sure it was returning green on this PR the entire time, which is why we didn't fix this before it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.