core: change set internals and make (extreme) performance improvements #3992
Conversation
|
@phinze I know we're in the last miles towards a new release, but I believe this PR should be in there. Please see the description for more details, or ping me if you want to discuss this in more detail... Thx! |
|
This looks like some great work, @svanharmelen! However, I think a deeper core change like this wouldn't be responsible for us to merge it this late in the release cycle. We can land it first thing after 0.6.7 is cut so that it can have some time on master to bake before it makes it into a release. I know you were hoping to get this in for 0.6.7, but I hope the conservative play makes sense to you here! |
|
Your absolutely right... And thinking about it again, I think I shouldn't have suggested to put it in right now in the first place. So yes, it makes sense 😉 |
Changing the Set internals makes a lot of sense as it saves doing conversions in multiple places and gives a central place to alter the key when a item is computed. This will have no side effects other then that the ordering is now based on strings instead on integers, so the order will be different. This will however have no effect on existing configs as these will use the individual codes/keys and not the ordering to determine if there is a diff or not. Lastly (but I think also most importantly) there is a fix in this PR that makes diffing sets extremely more performand. Before a full diff required reading the complete Set for every single parameter/attribute you wanted to diff, while now it only gets that specific parameter. We have a use case where we have a Set that has 18 parameters and the set consist of about 600 items (don't ask 😉). So when doing a diff it would take 100% CPU of all cores and stay that way for almost an hour before being able to complete the diff. Debugging this we learned that for retrieving every single parameter it made over 52.000 calls to `func (c *ResourceConfig) get(..)`. In this function a slice is created and used only for the duration of the call, so the time needed to create all needed slices and on the other hand the time the garbage collector needed to clean them up again caused the system to cripple itself. Next to that there are also some expensive reflect calls in this function which also claimed a fair amount of CPU time. After this fix the number of calls needed to get a single parameter dropped from 52.000+ to only 2! 😃
|
@phinze is now a good time to merge this one in? Or do you expect a "hotfix" release within the next couple of days? |
|
@svanharmelen Ha I just looked at this issue about an hour ago and had the same thought. I think we'll get a "hotfix" release out soon to address a few reported 0.6.7 bugs then we'll pull this in. 👍 |
|
@phinze check! |
|
@phinze I would be really nice if we can get this one in now... |
|
@svanharmelen indeed! This seems good to me, but I'd like to get @mitchellh to review as well just for a double check. |
|
All of this looks fairly straightforward as caching except for the changes to the compute parts. However, if all the unit tests pass here I feel fairly confident in this, but we should verify with some acceptance tests around things that use sets as well. |
|
@phinze @mitchellh by all means verify the changes with running a few acceptance tests 😃 We are using this code internally for a couple of weeks now without any issues. Most noticeable with all the firewall related CloudStack resources (which all have sets), so personally I'm confident this change is not breaking stuff. But of course good to double check with some other resources as well... So if you could spin up some tests and let me know it went, that would be awesome! |
|
Just ran all AWS Security Group tests on this which are pretty Time to 🚢 😀 |
core: change set internals and make (extreme) performance improvements
|
Nice... Thx! |
|
|
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Changing the Set internals makes a lot of sense as it saves doing conversions in multiple places and gives a central place to alter the key when a item is computed.
This will have no side effects other then that the ordering is now based on strings instead on integers, so the order will be different. This will however have no effect on existing configs as these will use the individual codes/keys and not the ordering to determine if there is a diff or not.
Lastly (but I think also most importantly) there is a fix in this PR that makes diffing sets extremely more performand. Before a full diff required reading the complete Set for every single parameter/attribute
you wanted to diff, while now it only gets that specific parameter.
We have a use case where we have a Set that has 18 parameters and the set consist of about 600 items (don't ask 😉). So when doing a diff it would take 100% CPU of all cores and stay that way for almost an hour before being able to complete the diff.
Debugging this we learned that for retrieving every single parameter it made over 52.000 calls to
func (c *ResourceConfig) get(..). In this function a slice is created and used only for the duration of the call, so the time needed to create all needed slices and on the other hand the time the garbage collector needed to clean them up again caused the system to cripple itself. Next to that there are also some expensive reflect calls in this function which also claimed a fair amount of CPU time.After this fix the number of calls needed to get a single parameter dropped from 52.000+ to only 2! 😃