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

Conversion and deep-copy evolution #12598

Closed
uluyol opened this issue Aug 12, 2015 · 5 comments
Closed

Conversion and deep-copy evolution #12598

uluyol opened this issue Aug 12, 2015 · 5 comments
Labels
priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@uluyol
Copy link
Contributor

uluyol commented Aug 12, 2015

With the introduction of the experimental api prefix, we introduce large amounts of duplicate code for our autogenerated conversions and deep-copy functions. This increases the size of our binaries, and is not elegant. For simplicity, I will only talk about deep-copies, but everything that follows should also apply to conversions. I propose that we eliminate the duplicates as follows:

  • We remove all autogenerated deep-copy functions from the source tree whenever we generate new deep-copy functions (or use a build tag to hide them from the go compiler)
  • We check whether the Scheme has a deep-copy function for the target type already registered, if not, we generate a deep-copy function
  • Whenever we a see that a deep-copy function has already been generated, we do the copy by calling into the conversion.Cloner

(In the case of handwritten conversions, we will need to make them switch to using the conversion.Scope instead of calling autogenerated functions directly).

Assuming that we create the deep-copy functions in the order following the import dependency tree, we should not have any duplicate deep-copy functions. However, this approach brings with it some overhead.

For both cleaning up our deep-copy functions and increasing performance in the face of this overhead, I propose we take the following steps:

  • Profile the apiserver to measure what time is spent doing deep-copies
  • Create some microbenchmarks to use as a baseline (initial work can be found here)
  • Create a new method DeepCopyTo and pass pointers to avoid needless copies
  • Switch to exception handling within the deep-copies (elaborated below)
  • Change autogenerated functions to use generic signatures instead of per-type ones i.e. instead of f(a, b *T) error, we have f(a, b interface{}) and use type assertions. This lets us reduce the amount of reflection that we use in exchange for boxing

In Go, exception handling is typically discouraged, but it is completely reasonable to use it so long as it doesn't cross an API boundary. For example, here it is used within the standard library. When we see an error during a copy, the copy operation cannot recover. This is the prime use-case for exception handling. We can similarly catch errors within Scheme.DeepCopy, and instead of doing explicit error checking all over the place, simply panic and convert it into an error in DeepCopy. We can also create a new function, MustDeepCopy, which does not catch the error for use within the deep-copy functions.

@wojtek-t @nikhiljindal

@cjcullen cjcullen added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. team/api priority/backlog Higher priority than priority/awaiting-more-evidence. labels Aug 12, 2015
@wojtek-t
Copy link
Member

Regarding benchmars, there are some simple benchmarks here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/api/deep_copy_test.go
https://github.com/kubernetes/kubernetes/blob/master/pkg/api/conversion_test.go
They are both using not completely artificial data.

@uluyol
Copy link
Contributor Author

uluyol commented Aug 13, 2015

The main issue with those is that they use types in the api package, and are unlikely to recursively call into Scheme.

@uluyol
Copy link
Contributor Author

uluyol commented Aug 20, 2015

@caesarxuchao

@lavalamp
Copy link
Member

Change autogenerated functions to use generic signatures instead

IMO compiler-enforced type safety is a large part of why one would do auto-generated functions in the first place, so I don't agree with this.

Switch to exception handling within the deep-copies

Similarly, I don't see the need for this when we already have working errors. There's a cost to generating & catching panics; I don't see a particularly good reason why we should switch.

@uluyol
Copy link
Contributor Author

uluyol commented Dec 25, 2015

Most of this should be outdated now.

@uluyol uluyol closed this as completed Dec 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

4 participants