Code to automatically generate functions for DeepCopy

[wojtek-t]

With the new approach, DeepCopies are roughly 2-3x faster:

benchmark                              old ns/op     new ns/op     delta
BenchmarkPodCopy                       18331         6172          -66.33%
BenchmarkNodeCopy                      10135         5443          -46.30%
BenchmarkReplicationControllerCopy     18732         6469          -65.47%

This is currently just a proposal (no tests, etc.) - just let me know what do you think about it.

Ref #4521

cc @lavalamp @smarterclayton @krousey @timothysc

[wojtek-t]

cc @fgrzadkowski

[wojtek-t]

BTW - the second commit is the output from the generator, the first commit is the code that was really written.

[krousey]

Just a couple of notes on my part:

All the indent tracking in the generation seems unnecessary. Just run it through gofmt at the end.

Did you consider a method based approach instead of a registration approach? Something like

func (p *Pod) Copy() *Pod {
    // auto gen code
}

You could even add a generic interface for it.

type DeepCopier interface {
    DeepCopy() interface{}
}

func (p *Pod) DeepCopy() interface{} {
    // autogen code
}

func (p *Pod) Copy() *Pod { return p.DeepCopy().(*Pod) }

And when you're using some default reflection based DeepCopy, you can use this by adding a check to see if it satisfies this interface to get some optimization.

function deepCopy(src reflect.Value) reflect.Value {
    if src.Type().Implements(reflect.TypeOf(DeepCopier{})) {
        // call autogen code
    }
    // proceed with slow reflect code
}

Not sure if this is any better, but it doesn't require a central registrar just to know how to copy a type.

[smarterclayton]

We could probably make an exception on our "no methods on our value objects" for an explicit Clone() and Cloner interface.

Does that increase or reduce the complexity of this implementation?

----- Original Message -----

Just a couple of notes on my part:

All the indent tracking in the generation seems unnecessary. Just run it
through gofmt at the end.

Did you consider a method based approach instead of a registration approach?
Something like

func (p *Pod) Copy() *Pod {
  // auto gen code
}

You could even add a generic interface for it.

type DeepCopier interface {
  DeepCopy() interface{}
}

func (p *Pod) DeepCopy() interface{} {
  // autogen code
}

func (p *Pod) Copy() *Pod { return p.DeepCopy().(*Pod) }

And when you're using some default reflection based DeepCopy, you can use
this by adding a check to see if it satisfies this interface to get some
optimization.

function deepCopy(src reflect.Value) reflect.Value {
  if src.Type().Implements(reflect.TypeOf(DeepCopier{})) {
      // call autogen code
  }
  // proceed with slow reflect code
}

Not sure if this is any better, but it doesn't require a central registrar
just to know how to copy a type.

---

Reply to this email directly or view it on GitHub:
#8320 (comment)

[wojtek-t]

I thought about it and I don't think it will simplify the code. Why do you think it's better than this approach? I don't think readability should be an argument with respect to auto-generated code.

[krousey]

I guess I just prefer

p := q.Copy()

to

p := api.Scheme.DeepCopy(q)

You wouldn't need a conversion.Copier registry that relies on dispatching based on reflect.Type to a reflect.Value dynamically through it's Call() method. Also all of copier.go could just be deleted. Those are the upsides I see to this approach: both the calling code and implementation seem simpler.

[krousey]

Regarding readability (and my comment on the underscores), I agree that it's not as much of an issue in generated code. But at the same time, it still introduces non-zero cognitive overhead because it's checked in and might be read by someone. It'd be nice if it could be idiomatic and readable as well without too much effort.

[wojtek-t]

@krousey Thanks for the feedback. I get your points (although I don't think the implementation is significantly simpler).

@smarterclayton @thockin @bgrant0607 what do you think about the api suggested by @krousey in #8320 (comment)
As @smarterclayton pointed out above, we currently don't allow for methods on our value objects. I'm happy to change to the suggested approach, but we then need to agree on the exception from that rule.

[smarterclayton]

I would suggest we continue down our current path for now but consider it for post 1.0. There are a number of other proposals concerning our objects which are best dealt with together - using external structs from client libraries, separating out codecs from schemes, helpers for applying defaults, etc. I like the idea in theory, but it probably deserves consideration when we have the appropriate focus on it.

On May 18, 2015, at 6:58 PM, Wojciech Tyczynski notifications@github.com wrote:

@krousey Thanks for the feedback. I get your points (although I don't think the implementation is significantly simpler).

@smarterclayton @thockin @bgrant0607 what do you think about the api suggested by @krousey in #8320 (comment)
As @smarterclayton pointed out above, we currently don't allow for methods on our value objects. I'm happy to change to the suggested approach, but we then need to agree on the exception from that rule.

—
Reply to this email directly or view it on GitHub.

[thockin]

I'm a proponent of a major rethink about our API objects, but I think that is a bigger conversation that we want to have right now. Smaller steps is better

[wojtek-t]

Thanks for the feedback. I will then proceed with the approach that is already in this PR and leave a TODO to rethink it in the future.

Will ping this thread when it will be ready for review.

[bgrant0607]

With respect to the future, see #1451, #2306, and #7111, and issues those issues refer to.

[wojtek-t]

I've updated the PR - this is almost ready for final review (i.e. methods are already being generated correctly and there is a test). I will just do some few small changes tomorrow to it, but those will not affect the overall logic.

[wojtek-t]

The generated code is currently used only by the benchmark - I will change the call to use the new api in a subsequent PR.

This PR is now ready for the review - PTAL.

[wojtek-t]

Also - just to be clear - the second commit is the code that was auto-generated. The first commit is what was actually manuall written.

[krousey]

Overall it looks good to me. Just had one comment about the handling of maps and a couple of long term observations.

[wojtek-t]

@smarterclayton @lavalamp - can you please take a look?

[wojtek-t]

Thanks for taking a look during US holiday! @smarterclayton

All comments applied - PR ready for next pass of the review :)

[wojtek-t]

@smarterclayton - can you please take a look (or write that you're fine with the current version)? Thanks!

[smarterclayton]

LGTM

[lavalamp]

Your copy functions do not take pointers as their first arg... Why? Did you time that? go is making a shallow copy for you when you do that...

An alternate strategy, which may be faster, is to do a shallow copy and then recurse into things that need a deep copy (maps, pointers, slices). That sort of copy function would only need one parameter, a pointer to the thing it's supposed to expand, and it would involve a lot less memory copying.

[lavalamp]

My comments don't block this.

[wojtek-t]

Your copy functions do not take pointers as their first arg... Why? Did you time that? go is making a shallow copy for you when you do that...
An alternate strategy, which may be faster, is to do a shallow copy and then recurse into things that need a deep copy (maps, pointers, slices). That sort of copy function would only need one parameter, a pointer to the thing it's supposed to expand, and it would involve a lot less memory copying.

That's a very good point. Thanks for the suggestion.

However, if possible, I would like to do it in a separate PR.

[lavalamp]

Yeah, absolutely do a second PR. :)

[wojtek-t]

@piosz - please merge

[piosz]

Will wait for at least 3 green passes in a row on Jenkins.

[piosz]

Jenkins is still failing, but since it's a blocker we decided with @wojtek-t @fgrzadkowski to merge and look carefully into Jenkins.