fix(cache): update cache with O(1) data structures #780
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
bbatha
force-pushed
the
fix-cache-performance
branch
from
ed2a8c5 to
d97cfc8
Compare
k8s-ci-robot
added
size/L
k8s-ci-robot
added
the
do-not-merge/work-in-progress
src/cache_test.ts
Outdated
| expect(cache.get('name1')).to.equal(list[0]); | ||
| expect(cache.get('name2')).to.equal(list[1]); | ||
| expect(cache.get('name1', 'default')).to.equal(list[0]); | ||
| expect(cache.get('name2', 'default')).to.equal(list[1]); |
There was a problem hiding this comment.
I changed the test here to take the default namespace. The old logic didn't make much sense and it seemed like it was supposed to return undefined if the name and namespace did not match. From a consumer perspective I would expect the namespace to only be optional if the resource is global like a namespace or pv. Let me know I should restore the old behavior of finding the first object with a matching name regardless of what namespace its in if no namespace is provided.
There was a problem hiding this comment.
In the case of this test, the objects are global, they are V1Namespace so I think this test shouldn't change.
There was a problem hiding this comment.
Ah I see this test incorrectly puts namespace in the mocked responses so the namespace objects have namespaces. I'll split this into two tests one for namespace objects and one for unnamespaced objects.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
k8s-ci-robot
added
the
needs-rebase
The prior implementation used arrays to store cached objects. This meant that updates were O(n). In a controller I developed that managed ~70k PV and ~70k pvc 99.9% of CPU time was spent in cache updates pegging the entire process. This new implementation doesn't even have cache updates show up in the profiles and is using ~25m cpu for the same number of objects.
…th O(1) data structures
bbatha
force-pushed
the
fix-cache-performance
branch
from
15e74b9 to
a589f36
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/lgtm Thanks for the PR! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbatha, brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
The prior implementation used arrays to store cached objects. This meant
that updates were O(n). In a controller I developed that managed ~70k PV
and ~70k pvc 99.9% of CPU time was spent in cache updates pegging the
entire process. This new implementation doesn't even have cache updates
show up in the profiles and is using ~25m cpu for the same number of
objects.