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

Auto-generated conversion methods calling one another #7556

Merged
merged 2 commits into from May 7, 2015

Conversation

Projects
None yet
4 participants
@wojtek-t
Member

wojtek-t commented Apr 30, 2015

Part of #6800

Conversion time decreased by:

benchmark                                    old ns/op     new ns/op     delta
BenchmarkPodConversion                       149268        30690         -79.44%
BenchmarkNodeConversion                      60733         35517         -41.52%
BenchmarkReplicationControllerConversion     135528        32236         -76.21%

Comparing with no conversion methods:

benchmark                                    old ns/op     new ns/op     delta
BenchmarkPodConversion                       388502        30690         -92.10%
BenchmarkNodeConversion                      192973        35517         -81.59%
BenchmarkReplicationControllerConversion     385409        32236         -91.64%

@smarterclayton @lavalamp - your predictions of 10X improvements were so close to what it gives :)

cc @smarterclayton @lavalamp @fgrzadkowski

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Apr 30, 2015

Member

Note: Please do NOT merge #7560 and this PR at the same time - they need to be rebased before merging the other one.

Member

wojtek-t commented Apr 30, 2015

Note: Please do NOT merge #7560 and this PR at the same time - they need to be rebased before merging the other one.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 30, 2015

Contributor

Woohoo! What's the memory use look like?

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

Part of #6800

Conversion time decreased by:

benchmark                                    old ns/op     new ns/op
delta
BenchmarkPodConversion                       149268        30690
-79.44%
BenchmarkNodeConversion                      60733         35517
-41.52%
BenchmarkReplicationControllerConversion     135528        32236
-76.21%

Comparing with no conversion methods:

benchmark                                    old ns/op     new ns/op
delta
BenchmarkPodConversion                       388502        30690
-92.10%
BenchmarkNodeConversion                      192973        35517
-81.59%
BenchmarkReplicationControllerConversion     385409        32236
-91.64%

@smarterclayton @lavalamp - your predictions of 10X improvements were so
close to what it gives :)

cc @smarterclayton @lavalamp @fgrzadkowski
You can view, comment on, or merge this pull request online at:

#7556

-- Commit Summary --

  • Chain conversion functions while generation
  • Update conversion functions

-- File Changes --

M pkg/api/v1beta3/conversion.go (1603)
M pkg/conversion/converter.go (27)
M pkg/conversion/generator.go (79)

-- Patch Links --

https://github.com/GoogleCloudPlatform/kubernetes/pull/7556.patch
https://github.com/GoogleCloudPlatform/kubernetes/pull/7556.diff


Reply to this email directly or view it on GitHub:
#7556

Contributor

smarterclayton commented Apr 30, 2015

Woohoo! What's the memory use look like?

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

Part of #6800

Conversion time decreased by:

benchmark                                    old ns/op     new ns/op
delta
BenchmarkPodConversion                       149268        30690
-79.44%
BenchmarkNodeConversion                      60733         35517
-41.52%
BenchmarkReplicationControllerConversion     135528        32236
-76.21%

Comparing with no conversion methods:

benchmark                                    old ns/op     new ns/op
delta
BenchmarkPodConversion                       388502        30690
-92.10%
BenchmarkNodeConversion                      192973        35517
-81.59%
BenchmarkReplicationControllerConversion     385409        32236
-91.64%

@smarterclayton @lavalamp - your predictions of 10X improvements were so
close to what it gives :)

cc @smarterclayton @lavalamp @fgrzadkowski
You can view, comment on, or merge this pull request online at:

#7556

-- Commit Summary --

  • Chain conversion functions while generation
  • Update conversion functions

-- File Changes --

M pkg/api/v1beta3/conversion.go (1603)
M pkg/conversion/converter.go (27)
M pkg/conversion/generator.go (79)

-- Patch Links --

https://github.com/GoogleCloudPlatform/kubernetes/pull/7556.patch
https://github.com/GoogleCloudPlatform/kubernetes/pull/7556.diff


Reply to this email directly or view it on GitHub:
#7556

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Apr 30, 2015

Member

Woohoo! What's the memory use look like?

When I checked it in the morning the gain was significant but I can't remember it now. I will rerun the experiments tomorrow (I need to run now).

BTW PR rebased

Member

wojtek-t commented Apr 30, 2015

Woohoo! What's the memory use look like?

When I checked it in the morning the gain was significant but I can't remember it now. I will rerun the experiments tomorrow (I need to run now).

BTW PR rebased

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 30, 2015

Contributor

Can we move the generated functions to their own file conversion_generated.go?

Contributor

smarterclayton commented Apr 30, 2015

Can we move the generated functions to their own file conversion_generated.go?

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 1, 2015

Member

Awesome-- +1 for moving to a new file. I'll give a more detailed look later.

Member

lavalamp commented May 1, 2015

Awesome-- +1 for moving to a new file. I'll give a more detailed look later.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 1, 2015

Member

Can we move the generated functions to their own file conversion_generated.go?

I agree that's a good idea. However, since today is a public holiday in Poland I might not find time to do it before Monday.

Member

wojtek-t commented May 1, 2015

Can we move the generated functions to their own file conversion_generated.go?

I agree that's a good idea. However, since today is a public holiday in Poland I might not find time to do it before Monday.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 4, 2015

Member

Regarding memory, here is the comparison between having no generated conversions and this PR id ~3.5X better - the graphs below:

Before:
profile_old

After:
profile_new

Also, I've updated PR so that the generated code is in a separate file.

PTAL

Member

wojtek-t commented May 4, 2015

Regarding memory, here is the comparison between having no generated conversions and this PR id ~3.5X better - the graphs below:

Before:
profile_old

After:
profile_new

Also, I've updated PR so that the generated code is in a separate file.

PTAL

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 4, 2015

Member

BTW, once we agree that this is the final format, I will generate conversions to v1 api version in a separate PR.

Member

wojtek-t commented May 4, 2015

BTW, once we agree that this is the final format, I will generate conversions to v1 api version in a separate PR.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 4, 2015

Member

Update on memory
I think the the gain is bigger than 3.5X that i mentioned. Basically without generated conversion functions, there are 900 repetitions of methods, and with them generated 14.000 repetitions. So I think that we should multiply this 3.5X by 15 :) [or sth like that].

Member

wojtek-t commented May 4, 2015

Update on memory
I think the the gain is bigger than 3.5X that i mentioned. Basically without generated conversion functions, there are 900 repetitions of methods, and with them generated 14.000 repetitions. So I think that we should multiply this 3.5X by 15 :) [or sth like that].

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 4, 2015

Member

@lavalamp can you please take a look into it later today? Thanks!

Member

wojtek-t commented May 4, 2015

@lavalamp can you please take a look into it later today? Thanks!

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 4, 2015

Member

yes I will get to this by EOD

Member

lavalamp commented May 4, 2015

yes I will get to this by EOD

assignFormat := "if err := s.Convert(&in.%s, &out.%s, 0); err != nil {\n"
assignStmt := fmt.Sprintf(assignFormat, inField.Name, outField.Name)
if err := writeLine(w, indent, assignStmt); err != nil {
ifFormat := "if in.%s != nil {\n"

This comment has been minimized.

@lavalamp

lavalamp May 4, 2015

Member

I'm a little nervous that the else of this doesn't set out.%s = nil; out could be a pre-existing object...

@lavalamp

lavalamp May 4, 2015

Member

I'm a little nervous that the else of this doesn't set out.%s = nil; out could be a pre-existing object...

This comment has been minimized.

@wojtek-t

wojtek-t May 5, 2015

Member

Done (here are in few other places).

@wojtek-t

wojtek-t May 5, 2015

Member

Done (here are in few other places).

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 4, 2015

Member

So my major concern here is how does this interact with human defined conversion functions? When we make an API change, can I still write a conversion function like I could before? Or another way of putting it, can we turn this on for v1beta1/2?

Member

lavalamp commented May 4, 2015

So my major concern here is how does this interact with human defined conversion functions? When we make an API change, can I still write a conversion function like I could before? Or another way of putting it, can we turn this on for v1beta1/2?

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 4, 2015

Member

So my major concern here is how does this interact with human defined conversion functions? When we make an API change, can I still write a conversion function like I could before? Or another way of putting it, can we turn this on for v1beta1/2?

You definitely should be able to define the conversion function yourself (currently the conversion function wouldn't be generated if it's not obvious how to translate it - i.e. if there are field with different names, or of types that we couldn't automatically detect how to convert). In such cases, writing your own conversion function is obvious (it wouldn't conflict with anything)
The only question is what happens if translation could be detected automatically but we want the conversion to be done in a different way. But I think that we should block this PR on it.

Member

wojtek-t commented May 4, 2015

So my major concern here is how does this interact with human defined conversion functions? When we make an API change, can I still write a conversion function like I could before? Or another way of putting it, can we turn this on for v1beta1/2?

You definitely should be able to define the conversion function yourself (currently the conversion function wouldn't be generated if it's not obvious how to translate it - i.e. if there are field with different names, or of types that we couldn't automatically detect how to convert). In such cases, writing your own conversion function is obvious (it wouldn't conflict with anything)
The only question is what happens if translation could be detected automatically but we want the conversion to be done in a different way. But I think that we should block this PR on it.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 4, 2015

Member

I see. I was hoping we could read the manually written conversion functions, and use them to generate the expanded conversion functions.

The only question is what happens if translation could be detected automatically but we want the conversion to be done in a different way. But I think that we should block this PR on it.

I think this effectively will prevent us from mutating our api. We'll need a solution for this soon. I think we can still merge this if you have a plan to fix this soon.

Member

lavalamp commented May 4, 2015

I see. I was hoping we could read the manually written conversion functions, and use them to generate the expanded conversion functions.

The only question is what happens if translation could be detected automatically but we want the conversion to be done in a different way. But I think that we should block this PR on it.

I think this effectively will prevent us from mutating our api. We'll need a solution for this soon. I think we can still merge this if you have a plan to fix this soon.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 5, 2015

Member

I see. I was hoping we could read the manually written conversion functions, and use them to generate the expanded conversion functions.

My plan here is to change the manually written conversion functions to be named (with some explicitly defined naming convention) and the use them while generating conversion functions. I just didn't get to it yet.

I think this effectively will prevent us from mutating our api. We'll need a solution for this soon. I think we can still merge this if you have a plan to fix this soon.

Yes, I thought about it and I think it should be possible to do it pretty soon. Basically my plan is to:

  1. internally, store the generated and manually-written conversion functions separately
  2. in default (reflection-based) implementation, check whether any of those exists
  3. while generating conversion methods, if there is a manually-written conversion function, do NOT generate one for it
    It should be relatively simple PR and should fix the problem.
Member

wojtek-t commented May 5, 2015

I see. I was hoping we could read the manually written conversion functions, and use them to generate the expanded conversion functions.

My plan here is to change the manually written conversion functions to be named (with some explicitly defined naming convention) and the use them while generating conversion functions. I just didn't get to it yet.

I think this effectively will prevent us from mutating our api. We'll need a solution for this soon. I think we can still merge this if you have a plan to fix this soon.

Yes, I thought about it and I think it should be possible to do it pretty soon. Basically my plan is to:

  1. internally, store the generated and manually-written conversion functions separately
  2. in default (reflection-based) implementation, check whether any of those exists
  3. while generating conversion methods, if there is a manually-written conversion function, do NOT generate one for it
    It should be relatively simple PR and should fix the problem.
@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 5, 2015

Member

PTAL (hopefully the above explanation will satisfy you :))

Member

wojtek-t commented May 5, 2015

PTAL (hopefully the above explanation will satisfy you :))

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 5, 2015

Contributor

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

I see. I was hoping we could read the manually written conversion functions,
and use them to generate the expanded conversion functions.

My plan here is to change the manually written conversion functions to be
named (with some explicitly defined naming convention) and the use them
while generating conversion functions. I just didn't get to it yet.

I think this effectively will prevent us from mutating our api. We'll need a
solution for this soon. I think we can still merge this if you have a plan
to fix this soon.

Yes, I thought about it and I think it should be possible to do it pretty
soon. Basically my plan is to:

  1. internally, store the generated and manually-written conversion functions
    separately
  2. in default (reflection-based) implementation, check whether any of those
    exists
  3. while generating conversion methods, if there is a manually-written
    conversion function, do NOT generate one for it

Would it be an expectation on the manual-written conversion author to invoke the generated function by name? I.e.:

func myconversion() {
out.Bar = in.Bar
convert_api_SomeObject_to_v1beta3_SomeObject(out, in)
}

? If so, that seems reasonable.

Contributor

smarterclayton commented May 5, 2015

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

I see. I was hoping we could read the manually written conversion functions,
and use them to generate the expanded conversion functions.

My plan here is to change the manually written conversion functions to be
named (with some explicitly defined naming convention) and the use them
while generating conversion functions. I just didn't get to it yet.

I think this effectively will prevent us from mutating our api. We'll need a
solution for this soon. I think we can still merge this if you have a plan
to fix this soon.

Yes, I thought about it and I think it should be possible to do it pretty
soon. Basically my plan is to:

  1. internally, store the generated and manually-written conversion functions
    separately
  2. in default (reflection-based) implementation, check whether any of those
    exists
  3. while generating conversion methods, if there is a manually-written
    conversion function, do NOT generate one for it

Would it be an expectation on the manual-written conversion author to invoke the generated function by name? I.e.:

func myconversion() {
out.Bar = in.Bar
convert_api_SomeObject_to_v1beta3_SomeObject(out, in)
}

? If so, that seems reasonable.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 5, 2015

Member

Would it be an expectation on the manual-written conversion author to invoke the generated function by name? I.e.:
func myconversion() {
out.Bar = in.Bar
convert_api_SomeObject_to_v1beta3_SomeObject(out, in)
}
? If so, that seems reasonable.

That's exactly what I thought about.

Member

wojtek-t commented May 5, 2015

Would it be an expectation on the manual-written conversion author to invoke the generated function by name? I.e.:
func myconversion() {
out.Bar = in.Bar
convert_api_SomeObject_to_v1beta3_SomeObject(out, in)
}
? If so, that seems reasonable.

That's exactly what I thought about.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 5, 2015

Member

@lavalamp can you please take a look?

Member

wojtek-t commented May 5, 2015

@lavalamp can you please take a look?

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 5, 2015

Member

LGTM

Another future improvement would be to inline the defaulting code. It's making a lot of reflect.TypeOf calls right now.

Member

lavalamp commented May 5, 2015

LGTM

Another future improvement would be to inline the defaulting code. It's making a lot of reflect.TypeOf calls right now.

@lavalamp lavalamp added the lgtm label May 5, 2015

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 6, 2015

Member

Another future improvement would be to inline the defaulting code. It's making a lot of reflect.TypeOf calls right now.

Sure - but that's just an optimization.

Member

wojtek-t commented May 6, 2015

Another future improvement would be to inline the defaulting code. It's making a lot of reflect.TypeOf calls right now.

Sure - but that's just an optimization.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 6, 2015

Member

The #7814 showed how important is to have ability to support both manually created and automatically generated conversions together.
That's why I've already written #7832 for support both manually-written and auto-generated conversion functions.
I will rebase this PR only after #7832 is merged (this should be easier to do).

Member

wojtek-t commented May 6, 2015

The #7814 showed how important is to have ability to support both manually created and automatically generated conversions together.
That's why I've already written #7832 for support both manually-written and auto-generated conversion functions.
I will rebase this PR only after #7832 is merged (this should be easier to do).

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t May 7, 2015

Member

I've rebased this PR - the benchmark result are slightly worse now, because auto-generated functions are not explicitly calling manually-written ones - they are called implicitly via Convert() method.
However, the gain is still significant (~8.5X) and we can fix that in the future.

Tthere are no functional changes from the previous version - just regenerated conversion functions + slightly optimized the manually written ones.

PTAL

Member

wojtek-t commented May 7, 2015

I've rebased this PR - the benchmark result are slightly worse now, because auto-generated functions are not explicitly calling manually-written ones - they are called implicitly via Convert() method.
However, the gain is still significant (~8.5X) and we can fix that in the future.

Tthere are no functional changes from the previous version - just regenerated conversion functions + slightly optimized the manually written ones.

PTAL

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 7, 2015

Contributor

Going to merge

Contributor

smarterclayton commented May 7, 2015

Going to merge

smarterclayton added a commit that referenced this pull request May 7, 2015

Merge pull request #7556 from wojtek-t/conversions_with_defaulting
Auto-generated conversion methods calling one another

@smarterclayton smarterclayton merged commit b6fb8c8 into kubernetes:master May 7, 2015

4 checks passed

Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.72%) to 48.73%
Details
@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t
Member

wojtek-t commented May 7, 2015

Thanks @smarterclayton !

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 7, 2015

Member

we can fix that in the future.

SGTM :)

Member

lavalamp commented May 7, 2015

we can fix that in the future.

SGTM :)

@wojtek-t wojtek-t deleted the wojtek-t:conversions_with_defaulting branch May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment