Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dependency keys are not cleared on LRU eviction #497

Closed
vladar opened this issue Feb 28, 2023 · 2 comments · Fixed by #498 · May be fixed by #520
Closed

Dependency keys are not cleared on LRU eviction #497

vladar opened this issue Feb 28, 2023 · 2 comments · Fixed by #498 · May be fixed by #520

Comments

@vladar
Copy link
Contributor

vladar commented Feb 28, 2023 •
edited

Looks like there is a minor "leak" in optimism even with LRU enabled: entries of depsByKey always stay in memory even after all optimism Entries corresponding to the given key are cleared.

So this key + an empty dep Set are "leaking":

optimism/src/dep.ts

Lines 31 to 35 in 4301eac

let dep = depsByKey.get(key);
if (!dep) {
depsByKey.set(key, dep = new Set as Dep<TKey>);
}
parent.dependOn(dep);

In my testing it also survives Apollo eviction / GC, even cache.gc({ resetResultCache: true })
@vladar vladar mentioned this issue Mar 1, 2023
Merged
@vladar
Copy link
Contributor Author

vladar commented Mar 2, 2023 •
edited

Extensive testing in our app gave the following results for active usage scenario (with 5 iterations):

JS Heap
Without the fix (mb): 218, 221, 213, 220, 220 (avg 218.4 mb)
With the fix (mb):    208, 210, 209, 208, 202 (avg 207.4 mb)

@vladar
Copy link
Contributor Author

vladar commented Mar 14, 2023

Benchmark and repro: https://github.com/vladar/optimism-deps-fix

@vladar vladar mentioned this issue Apr 2, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@vladar