Editor: Cache block meta-sourcing for getBlock selector

[aduth]

This pull request seeks to optimize the implementation of getBlock, avoiding repeated iterations of a block's attributes to determine whether a meta-sourced attribute exists. Prior to these changes, every call to getBlock would iterate each of a block's attributes to determine whether it needed to provide a value from post meta. As there are very few blocks to which this behavior applies, it's largely wasted effort, particularly considering that, since the block type won't change, we especially need not consider block types after it can be determined for the first time that no meta-sourced attributes exist.

Implementation notes:

There are many different ways we could go about solving this general issue of attributes derivation for meta attributes. Previously I'd considered some larger refactorings of block attributes sourcing (#5077). This is something which isn't included here, and should still be considered separately (#4989). The changes here are a more direct remedy to the logic of the current implementation.

Even within such a direct solution, I'm open to suggestions for improvement. I'd considered a few options with code placement of the utilities, and optionally selectors within the blocks module to detect whether attributes matching certain predicates exist. It's worth noting that the foundational block has no consideration of meta sources; this is a concept introduced by the editor. Thus, the closest I had got to making this a block concern was by introducing a getAttributeNamesBySource selector, which was both needlessly specific and not as impactful (easily-cached) as what's considered here.

The advantage of using a WeakMap cache is that it doesn't require manual invalidation, and respects the assumed immutability of a block type once registered. The downside as implemented is that it's another layer of caching, and introduces a few new utility functions which don't fall neatly into existing patterns (hasMetaSourcedAttribute is not quite a selector, createWeakCache could stand alone as its own module).

Testing instructions:

There should be no impact on general use of the editor, namely those behaviors which touch getBlockAttributes (keypress in paragraph). Confirm also no regressions on the behavior of blocks whose attributes are sourced from meta.†

† No meta-sourced core blocks exist. You may consider this example Stars block, with revisions to bring it up to date (add source: 'meta' to block type definition)

[swissspidy]

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

[aduth]

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.

[aduth]

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.

[gziolo]

@aduth and @youknowriad - is it still something we want to land? We keep moving it between milestones so I'm wondering what is the reason.

[aduth]

I'm wondering what is the reason

Because nobody gives it a review 😄

I think it can be an impactful change for performance, but it feels a little wrong to be introducing some unmanaged side-effect state.

There was some recent related conversation with a reference to the work here at #13088 (comment) . What's described there feels more comfortable, but is also more difficult to implement. The work here was meant more as a band-aid fix. It could prove valuable as a temporary measure if #13088 is meant to be pushed through in a rush, but otherwise I think in our current position we're not as stressed for this to be a solution when there are other, more-thought-out options available.

To that end, I might close it at least until we decide what we want to do with it.

[gziolo]

I'm wondering what is the reason

Because nobody gives it a review 😄

A pretty prosaic one :) These changes look complex so maybe that's why. In general, performance optimizations need a lot of time to be properly reviewed.