Sites: improve memoization of the getSites selector #19760

jsnajdr · 2017-11-14T16:14:58Z

A selector implemented using createSelector that has a state.sites.items dependenant will clear the memoization cache on every update of state.sites.items, even if only one site out of hundreds got updated and the rest are identical objects. That's inefficient. On a repeated getSite( state, id ), new site object will be computed, not ===-identical to the old one, and many components that depend on it will do an unneeded rerender.

This patch rewrites the getSite selector to memoize a site object in a WeakMap cache until the source rawSite object really changes.

The selector no longer depends on state.currentUser.capabilities, as that is a structure that fully depends on state.sites.items and is populated and changed only by actions that change state.sites.items at the same time.

There are awesome performance benefits when doing SitesList.sync during initial load. Executing the function is 3 times faster (1100ms to 350ms) and the number of rerenders is more than 10 times smaller (4700 to 380).

Fixes a big part of #19446.

How to test:

Put some console.log statements with timings around the this.sync( data ); in the fetch method in client/lib/sites-list/list.js.
Reload the page and check the timings with and without patch.
The "sympathy" step that avoids rehydrating the state from local storage has a big impact on the timing, too, so watch out for it.

matticbot · 2017-11-14T16:14:59Z

Test live: https://calypso.live/?branch=update/get-site-memoization

aduth · 2017-11-14T16:39:37Z

createSelector passes arguments to the dependents getter (second argument), so could the change have been instead to change from depending on the full state.sites.item to the singular site returned by the getRawSite / getSiteBySlug selectors?

samouri · 2017-11-14T17:00:54Z

so could the change have been instead to change from depending on the full state.sites.item to the singular site returned by the getRawSite / getSiteBySlug selectors?

nope!
I think this is a common mistake when using createSelector. There is only a single cache key for the whole cache, so if you make the second argument point to the specific site then you end up invalidating the cache way more often than you would expect. Heres an example:

1. getSite( siteId1 )
2. getSite( siteId2 ) // this request clears the cache
3. getSite( siteId1 ) // this ideally would use the cache if the site with siteId1 never changed

I have the issue documented pretty well here:

Also relevant is @edo-ran's attempt to invalidate the cache on a per-key basis: #12712.

This solution makes a lot of sense to me. Would be nice if we could generalize it too

blowery · 2017-11-14T17:15:34Z

Wooooooooooo

blowery · 2017-11-14T17:16:33Z

client/state/sites/selectors.js

shoud update the type too

blowery · 2017-11-14T17:17:03Z

client/state/sites/selectors.js

should this be above the docblock?

we can also give it its own JSDOC block comment indicating its use and contents

Docblock rearranged, added own docblock for getSiteCache

jsnajdr · 2017-11-14T17:39:13Z

so could the change have been instead to change from depending on the full state.sites.item to the singular site returned by the getRawSite / getSiteBySlug selectors?

That was actually the first thing I tried to do 🙂 But as @samouri said -- the memoization didn't work, because the cache is cleared every time the siteId arg changes.

samouri · 2017-11-14T18:46:39Z

I haven't been able to repro the slow this.sync( data) call :/

I placed a console.time('sync') before and a console.timeEnd('sync'); and the times are relatively similar.

Could this be due to that I only have 18 sites in my sidebar and the issue only crops up with more?
EDIT: I have 140 hidden.

apeatling · 2017-11-14T19:19:15Z

For context I have 345 sites, with 315 of them hidden.

jsnajdr · 2017-11-14T21:07:54Z

Could this be due to that I only have 18 sites in my sidebar and the issue only crops up with more?

In the case of SitePicker, the number of rerenders is n²/2 -- when n-th site is added, all 1..n are rerendered. This patch makes the growth linear.

I have 54 sites and the number of rerenders is ten times smaller after patching. If Andy's profile shows 16 seconds spent in SitesList.sync(), the unpatched render counts must be in the 100k to 300k range.

Given the quadratic growth, it sounds plausible that 18 sites are not enough to show a big difference.

samouri · 2017-11-15T00:13:24Z

This solution makes a lot of sense to me. Would be nice if we could generalize it too

I've taken a stab at a generalized solution by adding more fine grained cache-invalidation capabilities to createSelector over here. This WeakMap implementation has some space advantages though, so I wouldn't halt the progress of this PR for it!

gwwar · 2017-11-15T20:11:05Z

client/state/sites/selectors.js

Did we still need the comment about support for non-id site retrieval? Or should we rename the param siteId to siteIdOrSiteSlug

Renaming the param to siteIdOrSlug

gwwar · 2017-11-15T20:13:11Z

client/state/sites/selectors.js

Looks like the usage was there before, but do these selectors work properly if we pass through a siteSlug vs a siteId?

You're right, that's a pre-existing bug in the selector. I'll fix it before merging.

gwwar · 2017-11-15T20:24:53Z

client/state/sites/selectors.js

So tracing this down the state.sites.items array holds a hard reference to our rawSites. So in theory we should be able to GC weak refs in the map after state.sites.items is updated again, right?

the rawSite will be collected but the cache of it won't. as long as the actual rawSite reference is still in memory (such as in state.sites.items) then we'll be able to retrieve the value here. once it's no longer in use outside of this place it can be freed. at that point, we'll love our ability to retrieve the cached value; it's not clear to me if that value will leak or if the engine will free it as well

clearly I don't know what I'm doing here, but I tried to see if the weak map freed its local objects. in this ill-performed experiment i'm not sure what it indicates. but, the data is there for any more informed eyes

The WeakMap entry holds a weak reference to the key and a strong reference to the value. If anyone is holding a strong reference to the key, they can use it to access the value in the map.

If a reference to the key no longer exists, the entry becomes inaccessible and is removed from the map during the GC. During that GC, the strong reference to the value is removed. If it was the last reference to it, the value also becomes eligible for GC.

During that GC, the strong reference to the value is removed. If it was the last reference to it, the value also becomes eligible for GC.

This is what seems most expected to me, but I couldn't find the documentation for it; seems like it should be obvious.

gwwar

Thanks @jsnajdr for jumping on this. I confirmed that it's around 3 times faster. ❤️

Unless folks have objections, let's get this in and help folks on #19771 for a more generic fix. I also think a hand-crafted solutions are fine on known bottlenecks.

dmsnell · 2017-11-15T21:02:45Z

client/state/sites/selectors.js

how odd, WeakMap has no clear…

dmsnell · 2017-11-15T21:03:40Z

client/state/sites/selectors.js

maybe it's worth mentioning here at the top in the comment that this function depends on state in the selector module

samouri

I wasn't able to repro the slow case... but i can confirm it works and didn't get slower!

gwwar · 2017-11-15T22:12:50Z

I wasn't able to repro the slow case

😆 @samouri just test more nux flows!

dmsnell

Wish I had more cycles to dedicate to reviewing this but I don't ¯\_(ツ)_/¯ - don't let me hold you back.

A selector implemented using `createSelector` that has a `state.sites.items` dependenant will clear the memoization cache on every update of `state.sites.items`, even if only one site out of hundreds got updated and the rest are identical objects. That's inefficient. On a repeated `getSite( state, id )`, new site object will be computed, not `===`-identical to the old one, and many components that depend on it will do an unneeded rerender. This patch rewrites the `getSite` selector to memoize a site object in a `WeakMap` cache until the source `rawSite` object really changes. The selector no longer depends on `state.currentUser.capabilities`, as that is a structure that fully depends on `state.sites.items` and is populated and changed only by actions that change `state.sites.items` at the same time. There are awesome performance benefits when doing `SitesList.sync` during initial load. Executing the function is 3 times faster (1100ms to 350ms) and the number of rerenders is more than 10 times smaller (4700 to 380). Fixes a big part of #19446.

jsnajdr · 2017-11-16T12:05:25Z

I rebased the PR and fixed a few documentation and naming nits reported by reviewers.

I also fixed the bug reported by @gwwar where the computed attributes wouldn't be computed when getting the site by slug -- the slug would be passed to the get{Site,Jetpack}ComputedAttributes functions that only accept an id though. I added a test that verifies the fix.

Going to merge now.

beaulebens · 2017-11-21T21:24:35Z

@jsnajdr I'm not sure if it's the same issue, or a different one causing a similar symptom, but I'm still seeing a massive slowdown/unresponsiveness in the UI.

I've found a way to make it easily reproducible. A caveat here is that I can only spot it on an account with a lot of sites (over 400). I think on accounts with less sites it's maybe such a brief hang that I don't notice it while clicking around:

in the same browser/window, switch between staging/production. In my case I use ProxySwitchy, so it's just a matter of clicking a button.
after switching, refresh Calypso
start clicking around and within about 15 seconds you'll get the hang/interactions other than scrolling will halt
switch back to the other (production/staging) and refresh again
click around, you'll get the hang again

I guess switching environment is clearing something in localstorage(?) that's requiring whatever is causing the slowdown to happen again. Interestingly, trying to reproduce this by just clearing localstorage manually was not successful.

blowery · 2017-11-21T21:28:17Z

Interestingly, trying to reproduce this by just clearing localstorage manually was not successful.

If it's a storage issue, you'd need to clear both localStorage and indexed DB.

However, there's a decent chance the problem is in how we merge the sites-list we get from storage and the one we get from the api...

blowery · 2017-11-21T21:30:22Z

#19735 might be related

jsnajdr added [Type] Performance Sites [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 14, 2017

jsnajdr self-assigned this Nov 14, 2017

jsnajdr requested review from aduth, apeatling, dmsnell, gwwar, lamosty, ockham and samouri November 14, 2017 16:14

blowery reviewed Nov 14, 2017

View reviewed changes

gwwar added the Marked for Review Day label Nov 14, 2017

samouri mentioned this pull request Nov 14, 2017

createSelector: allow for per-key dependents #19771

Closed

4 tasks

gwwar reviewed Nov 15, 2017

View reviewed changes

gwwar approved these changes Nov 15, 2017

View reviewed changes

dmsnell reviewed Nov 15, 2017

View reviewed changes

client/state/sites/selectors.js Outdated

Copy link

Member

dmsnell Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how odd, WeakMap has no clear…

dmsnell reviewed Nov 15, 2017

View reviewed changes

samouri approved these changes Nov 15, 2017

View reviewed changes

dmsnell reviewed Nov 15, 2017

View reviewed changes

jsnajdr force-pushed the update/get-site-memoization branch from 2d4c534 to 6d299e3 Compare November 16, 2017 12:02

jsnajdr merged commit 6a2398a into master Nov 16, 2017

jsnajdr deleted the update/get-site-memoization branch November 16, 2017 13:11

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 16, 2017

Sites: improve memoization of the getSites selector #19760

Sites: improve memoization of the getSites selector #19760

Uh oh!

Conversation

jsnajdr commented Nov 14, 2017

Uh oh!

matticbot commented Nov 14, 2017

Uh oh!

aduth commented Nov 14, 2017

Uh oh!

samouri commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blowery commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsnajdr commented Nov 14, 2017

Uh oh!

samouri commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apeatling commented Nov 14, 2017

Uh oh!

jsnajdr commented Nov 14, 2017

Uh oh!

samouri commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwwar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samouri left a comment

Choose a reason for hiding this comment

Uh oh!

gwwar commented Nov 15, 2017

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

jsnajdr commented Nov 16, 2017

Uh oh!

beaulebens commented Nov 21, 2017

Uh oh!

blowery commented Nov 21, 2017

Uh oh!

blowery commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

samouri commented Nov 14, 2017 •

edited

Loading

samouri commented Nov 14, 2017 •

edited

Loading

samouri commented Nov 15, 2017 •

edited

Loading