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

Editor: Cache block meta-sourcing for getBlock selector #12555

Closed
wants to merge 2 commits into from
Closed

Editor: Cache block meta-sourcing for getBlock selector #12555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
545926c
Editor: Add createWeakCache utility
aduth Dec 3, 2018
3ee21cd
Editor: Cache block meta-sourcing for getBlock selector
aduth Dec 3, 2018
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
18 changes: 17 additions & 1 deletion packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { addQueryArgs } from '@wordpress/url';
*/
import { PREFERENCES_DEFAULTS } from './defaults';
import { EDIT_MERGE_PROPERTIES } from './constants';
import createWeakCache from '../utils/create-weak-cache';

/***
* Module constants
Expand All @@ -66,6 +67,21 @@ const ONE_MINUTE_IN_MS = 60 * 1000;
*/
const EMPTY_ARRAY = [];

/**
* Given a block type object, returns true if an attribute value is sourced
* from a meta property, or false otherwise.
*
* The value is cached weakly by strict object reference to the provided block
* type argument.
*
* @param {WPBlockType} blockType Block type against which to test.
*
* @return {boolean} Whether block type has an meta-sourced attribute.
*/
const hasMetaSourcedAttribute = createWeakCache( ( blockType ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can‘t we just use memoize instead of rolling our own? We already use it elsewhere, e.g. the i18n package.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a small but important difference in how this works using WeakMap as the cache storage, where normal memoization would hold on to values in memory forever, this would allow garbage collection to occur.

The second inconvenience is a memory leak because the arrays ensure that references to each key and each value are maintained indefinitely. These references prevent the keys from being garbage collected, even if there are no other references to the object. This would also prevent the corresponding values from being garbage collected.

memoized = memoize( _.stubTrue );
weakCached = createWeakCache( _.stubTrue );

var obj = { a: 1 };
weakCached( obj );
// [ { a: 1 } => true ]
memoized( obj );
// [ { a: 1 } => true ]

obj = { b: 2 };

weakCached( obj );
// [ { b: 2 } => true ]
memoized( obj );
// [ { a: 1 } => true, { b: 2 } => true ]
//   ┃
//   ┗━ This will be stuck in memory forever, despite there being no other
//      reference to the object.

For blocks, we'd really only achieve some significant benefit if we expect blocks to be overwritten at any point. Practically speaking this probably wouldn't occur, so we could potentially be fine with a standard memoization, albeit with a very fragile guarantee against memory leaks.

rememo (createSelector) has some internal tricks to achieve a similar effect, so there might be some options there, but it'd be quite indirect.

Copy link
Member Author

@aduth aduth Dec 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Lodash's documentation includes a note about swapping its underlying cache implementation, though as described it appears to apply globally. Each memoized function has its own cache, so I'd wonder if it might be possible to assign to it directly.

https://lodash.com/docs/4.17.11#memoize

Maybe possible?

const hasMetaSourcedAttribute = memoize( ( blockType ) => { /* ... */ } );
hasMetaSourcedAttribute.cache = new WeakMap;

Looking at the implementation, I think this would work.

return some( blockType.attributes, { source: 'meta' } );
} );

/**
* Returns true if any past editor history snapshots exist, or false otherwise.
*
Expand Down Expand Up @@ -649,7 +665,7 @@ export const getBlockAttributes = createSelector(
// TODO: Create generic external sourcing pattern, not explicitly
// targeting meta attributes.
const type = getBlockType( block.name );
if ( type ) {
if ( type && hasMetaSourcedAttribute( type ) ) {
attributes = reduce( type.attributes, ( result, value, key ) => {
if ( value.source === 'meta' ) {
if ( result === attributes ) {
Expand Down
25 changes: 25 additions & 0 deletions packages/editor/src/utils/create-weak-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Returns a function which caches a computed value on a given object key. Due
* to its caching being achieved by WeakCache, the function must only accept a
* valid WeakMap key as its argument (objects, arrays). The function is only
* passed the key; any other arguments are discarded.
*
* @link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap
*
* @param {Function} getCacheValue Function to compute the cache value.
*
* @return {Function} Function whose computed value is weakly cached.
*/
function createWeakCache( getCacheValue ) {
const cache = new WeakMap();

return ( key ) => {
if ( ! cache.has( key ) ) {
cache.set( key, getCacheValue( key ) );
}

return cache.get( key );
};
}

export default createWeakCache;
41 changes: 41 additions & 0 deletions packages/editor/src/utils/test/create-weak-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Internal dependencies
*/
import createWeakCache from '../create-weak-cache';

describe( 'createWeakCache', () => {
const getNumKeys = jest.fn().mockImplementation( ( object ) => Object.keys( object ).length );
const getNumKeysCached = createWeakCache( getNumKeys );

beforeEach( () => {
getNumKeys.mockClear();
} );

it( 'should return the value from the function argument', () => {
const object = { a: 1 };

expect( getNumKeysCached( object ) ).toBe( 1 );
} );

it( 'should return the value from cache', () => {
const object = { a: 1 };

expect( getNumKeysCached( object ) ).toBe( 1 );
expect( getNumKeysCached( object ) ).toBe( 1 );
expect( getNumKeys ).toHaveBeenCalledTimes( 1 );
} );

it( 'should return the value from the function argument on new reference', () => {
expect( getNumKeysCached( { a: 1 } ) ).toBe( 1 );
expect( getNumKeysCached( { a: 1 } ) ).toBe( 1 );
expect( getNumKeys ).toHaveBeenCalledTimes( 2 );
} );

it( 'should throw on invalid key argument', () => {
expect( () => getNumKeysCached( undefined ) ).toThrow();
} );

it( 'should discard additional arguments', () => {
expect( createWeakCache( ( a, b ) => b || 'ok' )( {}, 10 ) ).toBe( 'ok' );
} );
} );