-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Code to automatically generate conversion methods #7107
Code to automatically generate conversion methods #7107
Conversation
What about memory use
|
@smarterclayton I didn't look at the memory usage yet. Do you have any specific tests on you mind? How would you like me to measure it? |
If you just set up a benchmark you can add --memprofile mem.out to your bench test run and then run go tool pprof ./testbinary em.out --alloc_space ----- Original Message -----
|
I suspect we're still getting hit by the cost to generate the conversion stack and manipulate it, as well as some reflects. Can you add a few benchmark tests to this so I can look at some profiles and we can have a standard metric? |
Thanks - will run it tomorrow.
I'm pretty sure that's true. But I think that we should start with something (the changes I've made are already giving huge improvement) and we can continue improvements after. I would like to make one step at a time.
So far I was using pkg/api/serialization_test.go. But I can add more. |
Having a few specific ones is good because we can specifically profile those and get apples-to-apples comparisons. ----- Original Message -----
|
Sure - that's why I was so far using always pkg/api/serialization_test.go. But I agree that having more than one is good. |
That one will give different results every time (subtly) because, while the rand has a consistent seed, the iteration order of the runtime.Scheme map is different. So each run is going to be generating different fuzz for different objects. ----- Original Message -----
|
return nil | ||
} | ||
|
||
func (g *generator) writeConversionForSlice(f *os.File, inField, outField reflect.StructField, indent int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golang comment: use io.Reader/io.Writer, not os.File as parameters-- helps with testing among other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Hm, I was kinda thinking more along the lines of something that totally replaced the conversion functions with one big conversion function, like maybe it can take the existing conversion functions as input and glue them all together into one big function... it's interesting that this already gets a 2x speed improvement. We really want to track allocations. I think we can get this to more like 10x improvement if we cut out even more of the conversion package. |
Yeah, good first step though. The nice thing is you can potentially chain these and turn them into static functions with names, then have them cal each other (if necessary).
|
Absolutely - see my TODO: |
Yes I completely agree - this was also my plan once this is done. |
f6a677c
to
d503049
Compare
In #7163 I've created some dedicated benchmarks. |
@smarterclayton |
is what I use. |
Also, use https://github.com/golang/tools/tree/master/cmd/benchcmp to generate a diff of your benchmark results. |
Having incremental steps is good - I'd rather have a couple of related optimization passes than one giant step. That potentially allows us to test independently as well as isolate bugs. ----- Original Message -----
|
Thanks - I completely agree that's why I decided to send it out and not wait e.g. another 2 weeks with more advanced changes. |
That's very good that reflection is still our biggest cost. That means every reflect we remove is dramatically increasing performance. |
I agree - there is still a lot of place for improvements here. |
BTW - I don't understand the shippable failure - from the shippable page it seems that all tests passed :) |
d503049
to
43d78e7
Compare
Benchmark comparison
The memory allocation graphs above also show ~66% improvement. The next step I'm going to work on is to change those conversion functions to be named and call each other without going through reflections wherever it's possible. @smarterclayton @lavalamp |
e2e test are also passing: |
Can you add something to the upgrading the API doc so that people know that when they add things to conversions that the code is generated? We need to have a process as well for when someone manually adds a conversion working around a bug in the generator, how does the next person know to go fix the generator? Do we force people to use the generator? |
Currently the generator is still in kind of "experimental" mode so I wouldn't expect people to use it. I'm going to improve it (e.g. named conversion functions calling each other, some separation between user-provided methods and those automatically generated, etc.) Also I'm going to add a test checking whether methods in conversion.go is what we would generate with the generator. Where exactly do you want me to put it? |
----- Original Message -----
More just concerned that someone won't know. We can punt on it until you've advanced further, that's fine with me.
|
So if it's ok for you, I would wait with adding any documentation etc. until it's in more advanced stage. Once we have tests (consistency between what generator is generating and what already exist) etc. I will add necessary documentation to. Do you have any other comments blocking the merge? |
Some code style fixes, no issues from me on structure |
43d78e7
to
0b383a3
Compare
Comments applied - PTAL |
0b383a3
to
7ed9b28
Compare
LGTM, will merge on green. |
Hold on this please, I would like to look at it first. |
) | ||
|
||
func init() { | ||
err := newer.Scheme.AddConversionFuncs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is correct that these are auto-generated, yes? In that case I do NOT think we should commit in this way. The reason is that the other conversion functions are manually written, and if we do this we'll have a mix of manually and automatically written functions.
So, my recommendation is that you separate this in some way. E.g., autogenerate a func AddGeneratedConversionFunctions()
, and call that first in init (and then it can be overridden with any manually generate functions.
I think this is very important because eventually the flow should go the other way-- you should produce the auto-generated stuff by gluing together the manually written stuff. So it's important to make the difference between the auto-generated and manually written code very obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - those are autogenerated functions.
I agree that eventually this should be done how you described it (or similarly). But I don't think it's not a big problem now, because I'm focused now on v1beta3 where we don't have manually written conversion functions. But I'm going to work more on it next week.
I've added AddGeneratedConversionFunctions() method to scheme and I'm using it - I hope that would be enough for you for now.
BTW - I would like it to be merged soon (I will be working on it more right after this PR) to unblock other experiments that Filip is also doing in the performance area.
7ed9b28
to
7f919a4
Compare
PTAL |
Comments addressed, will expect author to handle more of dan's feedback later :) |
Code to automatically generate conversion methods
@smarterclayton Thanks! Sure - I will address any additional comments in subsequent PR. |
) | ||
|
||
func init() { | ||
err := newer.Scheme.AddGeneratedConversionFuncs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followup PR can you add a test that these haven't been manually edited? Thanks :)
Part of #6800
There are two commits in this PR:
This code is in a very early stage - the goal is to get the initial feedback, so any comments [including - this approach is bad, we should use a different one :)] are welcome.
The initial benchmarks show that conversion (without parsing jsons) is ~2x faster.
cc @lavalamp @smarterclayton @fgrzadkowski @davidopp