-
Notifications
You must be signed in to change notification settings - Fork 510
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
Linear cache: Add new UpdateResources method to update multiple resources at once without doing a stow update and limit unneeded allocations when processing delta watches #546
Conversation
4e20941
to
e705a9d
Compare
Thanks for this PR! I'll review this first thing tomorrow. I just approved the run, can you go ahead and fix up the lint checks? |
e705a9d
to
e5aead0
Compare
Just pushed the fix for the lint (at least working locally) |
…urces at once without doing a stow update and limit unneeded allocations when processing delta watches Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
e5aead0
to
57289fa
Compare
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.
Just a few minor nits/questions. Overall looks great thanks for the enhancements!
…ses as the version of deleted objects is not kept anyway Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
8508d3c
to
3dbc920
Compare
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.
Looks great, thanks for the PR! Once Github fixes actions we'll let the build run then get this merged if all is well.
Would an owner be able to trigger the checks to confirm it is all good? |
I am an owner :) unfortunately the APIs are actually down: https://www.githubstatus.com/ Github seems to be having an outage. I'm watching that page to see when they get resolved. Sorry for the delay! |
My bad, didn't see that it was down, sorry about that! |
@valerian-roche can you push a dummy commit? I think CI needs a trigger. Some instructions here to do so: https://github.com/envoyproxy/go-control-plane/blob/main/CONTRIBUTING.md#triggering-ci-re-run-without-making-changes |
@valerian-roche just merged! Thanks again nice work |
Thanks for the fast review and merge, greatly appreciated! |
In use cases where linear cache is used with eds and delta watches, the user can currently only update the cache in two ways:
In our case the main issue switching to the second part is the cpu usage (and time spent) on updates when updating multiple resources (though with still num_updates << total_keys, e.g. 50 vs 5000) while multiple delta watches exist (~20)
We then end up spending more than 120ms of CPU (for ~50 updates/deletes) with profiles like (this is aggregated for updates between 0 and 50):
In order to improve this, two changes are in this PR:
After the fix, runtime of the update (only includes iterating through calls to UpdateResource/DeleteResource before and on call to UpdateResources after) is greatly improved and stable (blue line below)