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.

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

Use per-dependants-set WeakMap-based cache #1

Merged
merged 11 commits into from
Feb 12, 2018
Merged

Conversation

aduth
Copy link
Owner

@aduth aduth commented Feb 9, 2018

Related: WordPress/gutenberg#4955 (comment)
Related: Automattic/wp-calypso#20330
Related: Automattic/wp-calypso#19771
Related: Automattic/wp-calypso#20547

This pull request seeks to provide improved caching for "per-key" selectors, taking advantage of WeakMap's behavior to automatically manage cache cleanup via garbage collection of unreferenced objects. This is heavily influenced by the implementation of Calypso's tree-select module.

More information is included in the docblock of the newly-introduced getCache function, responsible for returning a cache specific to each dependants set when possible:

Returns the cache for a given dependants array. When possible, a WeakMap
will be used to create a unique cache for each set of dependants. This
is feasible due to the nature of WeakMap in allowing garbage collection
to occur on entries where the key object is no longer referenced. Since
WeakMap requires the key to be an object, this is only possible when the
dependant is object-like. The root cache is created as a hierarchy where
each top-level key is the first entry in a dependants set, the value a
WeakMap where each key is the next dependant, and so on. This continues
so long as the dependants are object-like. If no dependants are object-
like, then the cache is shared across all invocations.

Testing instructions:

Verify that unit tests pass:

npm test

Verify that I'm not totally off-base with this implementation 😆

cc @samouri, @dmsnell, @youknowriad

Can't be guaranteed to have require cache clearing between tests when requiring outside of lifecycle as in previous implementation
Cannot pass `createSelector` reference as value which is assigned only after its invoked. Instead pass placeholder which proxies through to implementation while test is being run.
@youknowriad
Copy link

Do you think the maxSize option still makes sense with this change? I think it reflects the maxSize per dependency set, so I guess it's not that useful anymore?

Otherwise, this looks good to me.

Can simply use base object cache type when not available, similar to in case where dependants set includes primitive type.

Also only need to shallow equal check dependants when not guaranteed uniqueness.
Difficult to support with per-dependant-set unique cache. May provide some value but would need to reimplement in a future release.
@aduth
Copy link
Owner Author

aduth commented Feb 11, 2018

Do you think the maxSize option still makes sense with this change? I think it reflects the maxSize per dependency set, so I guess it's not that useful anymore?

Good call. I've never found myself caring to use this option, and it seems like it'd be difficult to implement with this new approach, so I've dropped it for now.

In e8c5cc2, I made the WeakMap support optional, which also helped simplify how to handle case where dependants include a primitive type. Only in environments where WeakMap is supported or the dependants include a primitive type do we need to perform shallow equality on the last dependants, as otherwise we don't ever need to clear the cache (relying on garbage collection when no references to original objects).

I'm considering both of the above revisions non-breaking, since WeakMap is an optional enhancement, and the options argument is simply ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants