Comments (14)
More notes from Lenz and me:
acemarke : ok, questions:
- is this just for the "primitive args" case?
- how would we test this?
- what would an appropriate max size be?
Lenz Weber-Tronic : Not only.
- You could also have one or more objects that dont change (happens with a sub-selector) as the first arguments, those followed by primitives.
- This is how I test for cache collection - a weakRef together with my own expect(weakRef).toBeGarbageCollected https://github.com/apollographql/apollo-client/blob/7485662fa1c0e618ba6b95ffa82cebdcf2973fec/src/link/persisted-queries/__tests__/persisted-queries.test.ts#L154-L178
- I'd probably even make configuring that on a case-by-case basis required, but if not, I'd probably go for something 10-ish
acemarke : where's that toBeGarbageCollected coming from?
Lenz Weber-Tronic : https://github.com/apollographql/apollo-client/pull/11369/files#diff-83faa655149377a65d05a36458355e0a84925b36d1ebe017f68196c27a369670
acemarke : ahhhh, okay, that explains why it wasn't turning up in a GH search :)
eskimojo : love a custom matcher
Lenz Weber-Tronic : Need to run jest with a node --expose-gc for that to work
acemarke : I feel like there's still something I'm missing about the memory leak possibility. Any chance you could walk me through the possible sequence that leads to an actual leak?
Lenz Weber-Tronic :memoizedFunction(someObj,3,8)
will end you up with this data structure:
WeakMap([[someObj, Map([[3, Map([[8, result]])]])]])
and
memoizedFunction(5,3,8)
ends you up with
Map([[5, Map([[3, Map([[8, result]])]])]])
The first case is fine, assuming that
someObj
is your Redux state and it leaves memory soon.
The secondcase will hold onto result forever.
Lenz Weber-Tronic : Now, if in the first case,someObj
is already a derived value that just does never change, but3,8
change to5,8
you end up with
WeakMap([[someObj, Map([[3, Map([[8, firstResult]])], [4, Map([[8, nextResult]])]])]])
Lenz Weber-Tronic : and as long as
someObj
will not leave memory, those deeply nested strong maps will just grow infintely as the primitive arguments change.
acemarke : (I think I like Ruby-ish / fat-arrow notation better for expressing key/value pairs :) but okay, mostly following)
Lenz Weber-Tronic : So, worst case you have a Redux store that never changes (or a slice that is derived as the first argument of a sub-selector), but a component iterates in 10000 renders over 10000 items in a list, and creates a very complex and memory-heavy object every time
Lenz Weber-Tronic : Something that would get really bad with this kind of selector design would be a
selectShopItems(state, from, to) { return state.items.slice(from, to) }
Now imagine you have an endless scrolling list, the user scrolls down, and from and to increase with every scroll event slightly
Lenz Weber-Tronic : You'd have all the arrays 1-10, 2-11, 3-12, 4-13, ..., 10001-10010 in memory long after the user scrolled by, as long as state doesn't change.
Lenz Weber-Tronic : Even worse, for each of those arrays, two new maps would be created
from reselect.
Lenz's latest example:
import { weakMapMemoize, createSelectorCreator } from "reselect";
const memoize = createSelectorCreator(weakMapMemoize);
const selector = memoize(
(array: any[], from: number, to: number) => array,
(array: any[], from: number, to: number) => from,
(array: any[], from: number, to: number) => to,
(array: any[], from: number, to: number) => array.slice(from, to),
);
let state = Array(10000)
.fill(null)
.map((_, i) => i);
const initialResult = new WeakRef(selector(state, 2000, 2500));
for (let i = 0; i < 10000; i++) {
selector(state, i, i + 100);
}
const laterResult = selector(state, 2000, 2500);
console.log(
"results are still memoized?",
initialResult.deref() === laterResult,
);
In one sense that works as intended. But I think the point is there ought to be some limits.
from reselect.
Lenz tackled similar issues on the Apollo side here:
from reselect.
I really like that you provide this new function weakmapMemoize
, it can solve a bunch of issues and simplify a lot of code.
However I am worried about the fact that it is now the default memoize function (BTW it's not clear in the doc but clear in the release announcement). As soon as we're using a primitive value in a selector, and the result of this selector is used in other selectors, we may have a chain of leaks (this depends on the other arguments too of course). (I'm more concerned about the memoize
function than the argsMemoize
function, because in our app we mostly never use several arguments to selectors)
The result of a selector may also have a significant size and we may have relied on the fact that reselect keeps just one copy, and suddenly this changes. (Though I understand that's what a major update is for).
Of course we can always go back to the lruMemoize
function, but I wish weakmapMemoize
would not be the default, so that we could use it if we want, maybe try it and adopt it first, so that you could gather feedback during some time. It would be good to have some explicit code to create a createSelector
with the old behavior. If I understand properly this would be:
import { createSelectorCreator, lruMemoize } from 'reselect';
const createSelectorLru = createSelectorCreator({ memoize: lruMemoize, argsMemoize: lruMemoize });
Maybe such an export could be exported from reselect
for an easier migration?
Last bit of feedback: the memoize
function has a special status in that it can be specified as an argument to createSelectorCreator
directly, but now I think memoize
and argsMemoize
are sibling (in a sort), and therefore maybe we shouldn't be able to pass simply the memoize
function to createSelectorCreator
, and instead force the user to pass both functions. My concern is that as part of the update users will simply forget to specify the argsMemoize
function if they used this shape earlier, I'd assume they'll want to specify both if they specify one.
I recognize this comment isn't super constructive, feel free to delete it if you feel it's useless for you.
from reselect.
(This is adapted from this discord thread: https://discord.com/channels/102860784329052160/103538784460615680/1192629604284907651)
Problem
I've recently updated a project to RTK2. Everything seemed fine, but we started getting user reports of stutters and performance issues. After investigating deeper, it appears that weakMapMemoize
is the main driver of the problem.
Specifically, we are getting very frequent major GC events when before we had virtually none.
RTK2 without weakMapMemoize
It's possible to opt-out of weakMapMemoize
in most of RTK, except RTKQ uses it internally for its selectors.
To test the app without RTKQ using weakMapMemoize
, I patched reselect
, exporting lruMemoize
as weakMapMemoize
: psychedelicious@2250a2a
I also add a console.log
so we can be confident that we are using the patched reselect
when running the app. Then built RTK using this patched reselect
, and used it in our app.
Test Case
I've run the chrome profiler with 3 configurations, while I do the same action in the app, using the same persisted redux state.
This action updates a single small object in redux very rapidly. The problem shows up in other places, but due to the frequency of this particular action, it's very apparent here.
Test Results
Here are the configurations and screenshots of memory allocations from the profiler.
Stock RTK2 - weakMapMemoize
everywhere (for both memoize
and argsMemoize
):
Configured RTK2 - lruMemoize
where it's configurable (in our createSelectorCreator
s and createEntityAdapter
's getSelectors()
), and weakMapMemoize
for RTKQ only:
Patched reselect
with weakMapMemoize = lruMemoize
:
Observations
- With only
weakMapMemoize
, almost every tick has a major GC, and the peak memory usage is roughly twice that of the other two configs - With the RTKQ only using
weakMapMemoize
, we still get intermittent major GCs - With patched reselect, no major GCs, only minor
- The allocation pattern is distinctly different between configurations, and clearly the least intense with no
weakMapMemoize
Notes
-
We use
useMemo
for parameterized selectors, relying on closures instead of selector arguments. This has been totally fine until RTK2. -
Due to the depth of the component tree, we have a strong emphasis on memoizing everything. We are pretty strict with passing stable references as props to components and generally the react devtools show very few rerenders.
In our customized selectors, we use lodash's
isEqual
forresultEqualityCheck
. In testing, this has consistently resulted in better app performance than no equality check and more rerenders. YMMV but for our app it's worked fine. -
The app has a lot of user interaction state. Where possible, this lives in lightweight state mgmt (e.g.
nanostores
or react builtins), but in some places it must be in redux. The test case is in one of those places. In other areas of the app with less intensive redux usage, there are no performance issues with RTK2/weakMapMemoize
.
Minimal Repro / Profiles
I will work on a minimal repro project that uses the same redux patterns as our full app, but it may be a couple days before I can put this together. (the app is OSS if it's helpful to review a real-world example).
I'm also happy to share profiles/memory allocation timelines/etc.
Proposed Solution
I think it's totally fine for weakMapMemoize
to be the new default. But I also think we need to be able to control it in RTKQ. To that effect, my particular issue would be mitigated by what I think would be a simple change:
-
RTKQ's
createApi
accepts acreateSelector
function so we can customize the memoize behaviour. (I don't thinkinjectEndpoints
needs this, but, you know, when you give a mouse a cookie...) -
Note the different memory characteristics of the memoize functions in the docs (and migration guide)
from reselect.
@psychedelicious I'll be very interested to see the repro, but based on your explanation I have a suspicion that either the combination of useMemo
and selectors created with weakMapMemoize
or resultEqualityCheck
being isEqual
might be playing a role in contributing to the issue.
from reselect.
@aryaemami59 There's actually a separate issue I've not yet reported. resultEqualityCheck
doesn't work at all with weakMapMemoize
. edit: logged in #679, sorry I neglected to raise this when I first encountered the problem.
Repro (increment the counter): https://codesandbox.io/p/sandbox/dazzling-kapitsa-gtqrl5?file=%2Fsrc%2Fapp%2Fstore.js%3A12%2C5
When I upgraded to RTK2, I had to choose between using lruMemoize
and isEqual
, or weakMapMemoize
with no resultEqualityCheck
- so, in a way, it's definitely not a problem with weakMapMemoize
+ isEqual
.
Def could be related to useMemo
and weakMapMemoize
. I can't really imagine why that would be problematic, though... Wouldn't this just result in many size=1 weakmap caches that get GC'd the same as if they were all LRU caches?
Will work towards a repro to clarify.
from reselect.
@psychedelicious On first glance, one thing that can cause a lot of issues is having state => state
in your selectors.
https://reselect.js.org/usage/common-mistakes
from reselect.
@aryaemami59 I must have missed that memo (heh). Thanks for pointing that out - I've used state => state
as a convenient input selector for just about all of my selectors.
I just went through the whole app and converted all input selectors to be like state => state.slice
. There are no identity selectors now.
Unfortunately, this has had no discernable impact on the memory issue.
Also, using resultEqualityCheck
with weakMapMemoize
still causes a fatal runtime error.
from reselect.
Updates for my issue
- @aryaemami59 very kindly gave me some pointers on our usage of
reselect
which I've implemented:
- Do not use
state => state
as an input selector - Reduce
resultEqualityCheck
usage except when necessary (I used this for bulk primitive property access as a convenience)
These improvements save about 4ms per animation frame on my computer, which is relevant for us. Thanks Arya!
Unfortunately, the memory usage with weakMapMemoize
was not improved.
- We've got an RC out with the
reselect
patchweakMapMemoize = lruMemoize
and thereselect
usage fixes. Users report this resolves the performance regression (and then some). This is further confirmation thatweakMapMemoize
can be problematic for some use patterns.
from reselect.
@psychedelicious I've opened a PR reduxjs/redux-toolkit#4048 to allow customising the createSelector
instance used by RTKQ. Do you reckon you could give the build a try?
from reselect.
Since updating from reselect 4.1.7 to 5.1.0, we have noticed a memory leak issue on our node servers.
On that zabbix graph, the memory is released when node (v20.12.2) restart.
After investigating the Heap snapshot, I discovered multiple references to previous selector values, which led me to suspect that the change in reselect version is the cause of this issue.
As a temporary solution, I reverted back to using lruMemoize
as suggested in #635 (comment). However, I found it peculiar that weakmaps were never released.
from reselect.
@paulgreg just to check, when you say "weakmaps were never released", do you mean "why didn't the weakmaps let go of the references they were holding on to"?
from reselect.
@markerikson yes, exactly.
from reselect.
Related Issues (20)
- How to type redux state and selectors with readonly? HOT 3
- Consider dev mode checks for `x => x` result functions HOT 2
- More Reselect addons to investigate HOT 1
- Add identifiable information to dev mode check log messages HOT 3
- Documentation ignores links in the table of contents entries HOT 1
- Type loss in `createSelector` with inline function declarations passed as separate arguments
- lastResult.deref is not a function (it is undefined) HOT 6
- Better call stack for selector warnings HOT 11
- Unable to use `resultEqualityCheck` with `weakMapMemoize` HOT 3
- Incorrect weakMapMemoize alternative example using useCallback HOT 1
- using createSelector.withTypes prevents build HOT 7
- Question: Why can't we support `createAsyncSelector`? HOT 11
- TypeError: (0 , _reselect.createSelector) is not a function HOT 12
- `weakMapMemoize` with `resultEqualityCheck` is provided empty objects for first call. HOT 3
- Library do not work in Safari < 14.1 HOT 1
- Why the LRUCache implementation is using Array over the Doubly Linked List with Map? HOT 8
- Question: should OutputSelector be used as an InputSelector? HOT 3
- Current documentration loose article about passing parameters HOT 1
- First level of cascading memoization is broken when more props are passed HOT 4
- [Documentation/Support] using mapping the result of a selector with another selector. HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from reselect.