ACM-20689-Compress-ACM-Backend-Caches #4526
Conversation
Signed-off-by: John Swanke <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeswanke 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 |
Signed-off-by: John Swanke <[email protected]>
Signed-off-by: John Swanke <[email protected]>
|
There was a problem hiding this comment.
John,
This is a very big memory saving, and some neat coding techniques. I appreciated that you handled keys that are already a number!
I noted some concerns inline about the data we are stripping.
Further, I performed an experiment by commenting out the calls to compressResource and decompressResource from deflateResource and inflateResource, respectively. I did this on weekly, which doesn't have a lot of data on it, but perhaps we could test against a scale environment or load up the cluster more to test.
main branch:
INFO:memory, caches{ appCache:306 KB, eventCache:702 KB, clients:0, events:696 KB } = 1704 KB
with your changes:
INFO:memory, caches{ clients:0, appCache:72 KB, appDict:24 KB, eventCache:198 KB, eventDict:75 KB } = 369 KB
with only zlib compression:
INFO:memory, caches{ clients:0, appCache:136 KB, appDict:0 KB, eventCache:304 KB, eventDict:0 KB } = 440 KB
So, it looks to me like we get almost all the benefit with zlib compression alone, which reduces the code complexity and risks. Might also be worth looking at the numbers without zlib and with just the dictionary-based, in case your method is cheaper (in CPU cycles) than zlib.
Did you make any efforts to measure impact on compute requirements or initial loading time? I'm curious if having to inflate every individual event will have an impact, or if network speeds and client-side processing will still be the limiting factor.
| // filter out all but the 3 most recent conditions | ||
| if (parentKey === 'conditions') { | ||
| resource = resource | ||
| .sort((a: SortTimeType, b: SortTimeType) => { | ||
| return new Date(b?.lastTransitionTime).getTime() - new Date(a?.lastTransitionTime).getTime() | ||
| }) | ||
| .slice(0, 3) | ||
| } |
There was a problem hiding this comment.
I'm not confident this is a safe change to make. Status conditions are used a lot in the UI code.
| } | ||
|
|
||
| // keys that point to values we should filter out | ||
| const filterKeys = new Set(['finalizers', 'flags']) |
There was a problem hiding this comment.
We already have a pruneResources function that strips certain things. It looks to me like these key names would apply recursively, and who knows where "flags" might appear in resource YAML. I think it would be better to centralize any pruning of data and to be explicit about what exact field we are targetting.
compression.ts now has three methods to compress the backend caches (event and app)
1. filter
if a resource has any of these keys it will filter it from cached resource:
const filterKeys = new Set(['finalizers', 'flags'])2. json compression
based on an idea in https://www.npmjs.com/package/compress-json
converts commonly used keys into a dictionary array (index=1, key='apiVersion') so that something lie this:
apiVersion: "cluster.open-cluster-management.io/v1beta1"is compressed to:
"1": "3"3. deflate
we then use the zlib deflateRawSync( ) function to zip this compressed json
before:
after:
Signed-off-by: John Swanke [email protected]