Skip to content

Sites: improve memoization of the getSites selector #19760

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

Merged
jsnajdr merged 1 commit into from
Nov 16, 2017
Merged

Sites: improve memoization of the getSites selector #19760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 32 additions & 25 deletions client/state/sites/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import {
assign,
compact,
every,
filter,
Expand Down Expand Up @@ -68,36 +67,44 @@ export const getSiteBySlug = createSelector(
);

/**
* Returns a normalized site object by its ID. Intends to replicate
* the site object returned from the legacy `sites-list` module.
*
* Memoization cache for the `getSite` selector
*/
let getSiteCache = new WeakMap();

/**
* Returns a normalized site object by its ID or site slug.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {Number|String} siteIdOrSlug Site ID or site slug
* @return {?Object} Site object
*/
export const getSite = createSelector(
( state, siteId ) => {
let site =
getRawSite( state, siteId ) ||
// Support for non-ID site retrieval
// Replaces SitesList#getSite
getSiteBySlug( state, siteId );

if ( ! site ) {
return null;
}
export function getSite( state, siteIdOrSlug ) {
const rawSite = getRawSite( state, siteIdOrSlug ) || getSiteBySlug( state, siteIdOrSlug );
if ( ! rawSite ) {
return null;
}

// To avoid mutating the original site object, create a shallow clone
// before assigning computed properties
site = { ...site };
assign( site, getSiteComputedAttributes( state, siteId ) );
assign( site, getJetpackComputedAttributes( state, siteId ) );
// Use the rawSite object itself as a WeakMap key
const cachedSite = getSiteCache.get( rawSite );
if ( cachedSite ) {
return cachedSite;
}

return site;
},
state => [ state.sites.items, state.currentUser.capabilities ]
);
const site = {
...rawSite,
...getSiteComputedAttributes( state, rawSite.ID ),
...getJetpackComputedAttributes( state, rawSite.ID ),
};

// Once the `rawSite` object becomes outdated, i.e., state gets updated with a newer version
// and no more references are held, the key will be automatically removed from the WeakMap.
getSiteCache.set( rawSite, site );
Copy link
Contributor

@gwwar gwwar Nov 15, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@dmsnell dmsnell Nov 15, 2017

Choose a reason for hiding this comment

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

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

screen shot 2017-11-15 at 4 20 36 pm

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

Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dmsnell dmsnell Nov 15, 2017

Choose a reason for hiding this comment

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

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.

return site;
}

getSite.clearCache = () => {
getSiteCache = new WeakMap();
Copy link
Member

@dmsnell 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…

};

export function getJetpackComputedAttributes( state, siteId ) {
if ( ! isJetpackSite( state, siteId ) ) {
Expand Down
108 changes: 88 additions & 20 deletions client/state/sites/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe( 'selectors', () => {
const stateWithNoItems = createStateWithItems( {} );

beforeEach( () => {
getSite.memoizedSelector.cache.clear();
getSite.clearCache();
getSiteCollisions.memoizedSelector.cache.clear();
getSiteBySlug.memoizedSelector.cache.clear();
} );
Expand Down Expand Up @@ -134,29 +134,29 @@ describe( 'selectors', () => {
} );

test( 'should return a normalized site with computed attributes', () => {
const site = getSite(
{
...userState,
sites: {
items: {
2916284: {
ID: 2916284,
name: 'WordPress.com Example Blog',
URL: 'https://example.com',
options: {
unmapped_url: 'https://example.wordpress.com',
},
const jetpackMinVersion = config( 'jetpack_min_version' );
const state = {
...userState,
sites: {
items: {
2916284: {
ID: 2916284,
name: 'WordPress.com Example Blog',
URL: 'https://example.com',
jetpack: true,
options: {
jetpack_version: jetpackMinVersion,
unmapped_url: 'https://example.wordpress.com',
},
},
},
siteSettings: {
items: {},
},
},
2916284
);
siteSettings: {
items: {},
},
};

expect( site ).to.eql( {
const expectedSite = {
ID: 2916284,
name: 'WordPress.com Example Blog',
URL: 'https://example.com',
Expand All @@ -166,11 +166,33 @@ describe( 'selectors', () => {
hasConflict: false,
is_customizable: false,
is_previewable: true,
jetpack: true,
hasMinimumJetpackVersion: true,
canAutoupdateFiles: false,
canUpdateFiles: false,
canManage: true,
isMainNetworkSite: false,
isSecondaryNetworkSite: true,
isSiteUpgradeable: null,
options: {
jetpack_version: jetpackMinVersion,
default_post_format: 'standard',
unmapped_url: 'https://example.wordpress.com',
},
} );
};

const site = getSite( state, 2916284 );
expect( site ).to.eql( expectedSite );

// Verify that getting by slug returns the object memoized when previously getting by ID
const memoizedSlugSite = getSite( state, 'example.com' );
expect( memoizedSlugSite ).to.equal( site );

// Clear the memo cache and verify computed attributes are computed when getting by slug
getSite.clearCache();
const nonMemoizedSlugSite = getSite( state, 'example.com' );
expect( nonMemoizedSlugSite ).to.not.equal( memoizedSlugSite );
expect( nonMemoizedSlugSite ).to.eql( expectedSite );
} );

test( 'should return a normalized site with correct slug when sites with collisions are passed in attributes', () => {
Expand Down Expand Up @@ -223,6 +245,52 @@ describe( 'selectors', () => {
},
} );
} );

test( 'should return an identical memoized site object when called for second time', () => {
const site = {
ID: 123,
name: 'Example Blog',
URL: 'https://example.wordpress.com',
};
const state = {
...userState,
sites: {
items: {
[ site.ID ]: site,
},
},
};

// Calling the selector two times on the same state should return identical value
const firstSite = getSite( state, 123 );
const secondSite = getSite( state, 123 );
expect( firstSite ).to.be.ok;
expect( secondSite ).to.be.ok;
expect( firstSite ).to.equal( secondSite );

// Construct an updated state with new items, but the first site object itself is unmodified
const altSite = {
ID: 456,
name: 'Alternative Blog',
URL: 'https://alt.wordpress.com',
};
const updatedState = {
...userState,
sites: {
items: {
[ site.ID ]: site,
[ altSite.ID ]: altSite,
},
},
};
// Check that the new site is returned
const altGotSite = getSite( updatedState, 456 );
expect( altGotSite ).to.have.property( 'ID', 456 );

// And that the old one was memoized and identical site object is returned
const thirdSite = getSite( updatedState, 123 );
expect( thirdSite ).to.equal( firstSite );
} );
} );

describe( '#getSiteCollisions', () => {
Expand Down