From 7a5dfe6fdaba68449a147e6f4d6ed10ce9f85999 Mon Sep 17 00:00:00 2001 From: Ian Storm Taylor Date: Fri, 5 May 2017 17:44:53 -0700 Subject: [PATCH] allow memoization without arguments for faster lookups (#796) --- src/models/mark.js | 4 +- src/models/node.js | 45 ++++++++------- src/models/text.js | 13 ++++- src/utils/memoize.js | 129 +++++++++++++++++++++++------------------- test/index.js | 2 +- test/plugins/index.js | 2 +- 6 files changed, 112 insertions(+), 83 deletions(-) diff --git a/src/models/mark.js b/src/models/mark.js index 60ce2e9af..123e108ea 100644 --- a/src/models/mark.js +++ b/src/models/mark.js @@ -77,7 +77,9 @@ class Mark extends new Record(DEFAULTS) { memoize(Mark.prototype, [ 'getComponent', -]) +], { + takesArguments: true, +}) /** * Export. diff --git a/src/models/node.js b/src/models/node.js index 8a7767579..06c6c92eb 100644 --- a/src/models/node.js +++ b/src/models/node.js @@ -1806,16 +1806,35 @@ const Node = { */ memoize(Node, [ - 'areDescendantsSorted', - 'getAncestors', 'getBlocks', 'getBlocksAsArray', + 'getCharacters', + 'getCharactersAsArray', + 'getFirstText', + 'getInlines', + 'getInlinesAsArray', + 'getKeys', + 'getLastText', + 'getMarks', + 'getOrderedMarks', + 'getMarksAsArray', + 'getText', + 'getTextDirection', + 'getTexts', + 'getTextsAsArray', + 'isLeafBlock', + 'isLeafInline', +], { + takesArguments: false +}) + +memoize(Node, [ + 'areDescendantsSorted', + 'getAncestors', 'getBlocksAtRange', 'getBlocksAtRangeAsArray', 'getBlocksByType', 'getBlocksByTypeAsArray', - 'getCharacters', - 'getCharactersAsArray', 'getCharactersAtRange', 'getCharactersAtRangeAsArray', 'getChild', @@ -1831,23 +1850,15 @@ memoize(Node, [ 'getDescendant', 'getDescendantAtPath', 'getDescendantDecorators', - 'getFirstText', 'getFragmentAtRange', 'getFurthestBlock', 'getFurthestInline', 'getFurthestAncestor', 'getFurthestOnlyChildAncestor', - 'getInlines', - 'getInlinesAsArray', 'getInlinesAtRange', 'getInlinesAtRangeAsArray', 'getInlinesByType', 'getInlinesByTypeAsArray', - 'getKeys', - 'getLastText', - 'getMarks', - 'getOrderedMarks', - 'getMarksAsArray', 'getMarksAtRange', 'getOrderedMarksAtRange', 'getMarksAtRangeAsArray', @@ -1865,11 +1876,7 @@ memoize(Node, [ 'getPreviousBlock', 'getPreviousSibling', 'getPreviousText', - 'getText', 'getTextAtOffset', - 'getTextDirection', - 'getTexts', - 'getTextsAsArray', 'getTextsAtRange', 'getTextsAtRangeAsArray', 'hasChild', @@ -1877,10 +1884,10 @@ memoize(Node, [ 'hasNode', 'hasVoidParent', 'isInlineSplitAtRange', - 'isLeafBlock', - 'isLeafInline', 'validate', -]) +], { + takesArguments: true +}) /** * Export. diff --git a/src/models/text.js b/src/models/text.js index 1777ee7bb..54cef26b5 100644 --- a/src/models/text.js +++ b/src/models/text.js @@ -407,14 +407,21 @@ class Text extends new Record(DEFAULTS) { */ memoize(Text.prototype, [ - 'getDecorations', - 'getDecorators', 'getMarks', 'getMarksAsArray', +], { + takesArguments: false, +}) + +memoize(Text.prototype, [ + 'getDecorations', + 'getDecorators', 'getMarksAtIndex', 'getRanges', 'validate' -]) +], { + takesArguments: true, +}) /** * Export. diff --git a/src/utils/memoize.js b/src/utils/memoize.js index 5f57125a8..e7096b786 100644 --- a/src/utils/memoize.js +++ b/src/utils/memoize.js @@ -3,7 +3,7 @@ import Map from 'es6-map' import IS_DEV from '../constants/is-dev' /** - * GLOBAL: True if memoization should is enabled. Only effective in DEV mode. + * GLOBAL: True if memoization should is enabled. Only effective when `IS_DEV`. * * @type {Boolean} */ @@ -12,7 +12,7 @@ let ENABLED = true /** * GLOBAL: Changing this cache key will clear all previous cached results. - * Only effective in DEV mode. + * Only effective when `IS_DEV`. * * @type {Number} */ @@ -20,9 +20,8 @@ let ENABLED = true let CACHE_KEY = 0 /** - * The leaf node of a cache tree. Used to support variable argument length. - * - * A unique object, so that native Maps will key it by reference. + * The leaf node of a cache tree. Used to support variable argument length. A + * unique object, so that native Maps will key it by reference. * * @type {Object} */ @@ -30,8 +29,8 @@ let CACHE_KEY = 0 const LEAF = {} /** - * A value to represent a memoized undefined value. Allows efficient - * value retrieval using Map.get only. + * A value to represent a memoized undefined value. Allows efficient value + * retrieval using Map.get only. * * @type {Object} */ @@ -54,7 +53,9 @@ const UNSET = undefined * @return {Record} */ -function memoize(object, properties) { +function memoize(object, properties, options = {}) { + const { takesArguments = true } = options + for (const property of properties) { const original = object[property] @@ -64,63 +65,50 @@ function memoize(object, properties) { object[property] = function (...args) { if (IS_DEV) { - if (!ENABLED) { - // Memoization disabled - return original.apply(this, args) - } else if (CACHE_KEY !== this.__cache_key) { - // Previous caches must be cleared + // If memoization is disabled, call into the original method. + if (!ENABLED) return original.apply(this, args) + + // If the cache key is different, previous caches must be cleared. + if (CACHE_KEY !== this.__cache_key) { this.__cache_key = CACHE_KEY this.__cache = new Map() } } - const keys = [property, ...args] - this.__cache = this.__cache || new Map() + if (!this.__cache) { + this.__cache = new Map() + } - const cachedValue = getIn(this.__cache, keys) + let cachedValue + let keys + + if (takesArguments) { + keys = [property, ...args] + cachedValue = getIn(this.__cache, keys) + } else { + cachedValue = this.__cache.get(property) + } + + // If we've got a result already, return it. if (cachedValue !== UNSET) { return cachedValue === UNDEFINED ? undefined : cachedValue } + // Otherwise calculate what it should be once and cache it. const value = original.apply(this, args) - this.__cache = setIn(this.__cache, keys, value) + const v = value === undefined ? UNDEFINED : value + + if (takesArguments) { + this.__cache = setIn(this.__cache, keys, v) + } else { + this.__cache.set(property, v) + } + return value } } } -/** - * Set a value at a key path in a tree of Map, creating Maps on the go. - * - * @param {Map} map - * @param {Array} keys - * @param {Any} value - * @return {Map} - */ - -function setIn(map, keys, value) { - value = value === undefined ? UNDEFINED : value - - let parentMap = map - let childMap - for (const key of keys) { - childMap = parentMap.get(key) - - // If the path was not created yet... - if (childMap === UNSET) { - childMap = new Map() - parentMap.set(key, childMap) - } - - parentMap = childMap - } - - // The whole path has been created, so set the value to the bottom most map. - childMap.set(LEAF, value) - - return map -} - /** * Get a value at a key path in a tree of Map. * @@ -133,18 +121,42 @@ function setIn(map, keys, value) { */ function getIn(map, keys) { - let childMap for (const key of keys) { - childMap = map.get(key) - - if (childMap === UNSET) { - return UNSET - } - - map = childMap + map = map.get(key) + if (map === UNSET) return UNSET } - return childMap.get(LEAF) + return map.get(LEAF) +} + +/** + * Set a value at a key path in a tree of Map, creating Maps on the go. + * + * @param {Map} map + * @param {Array} keys + * @param {Any} value + * @return {Map} + */ + +function setIn(map, keys, value) { + let parent = map + let child + + for (const key of keys) { + child = parent.get(key) + + // If the path was not created yet... + if (child === UNSET) { + child = new Map() + parent.set(key, child) + } + + parent = child + } + + // The whole path has been created, so set the value to the bottom most map. + child.set(LEAF, value) + return map } /** @@ -155,6 +167,7 @@ function getIn(map, keys) { function __clear() { CACHE_KEY++ + if (CACHE_KEY >= Number.MAX_SAFE_INTEGER) { CACHE_KEY = 0 } diff --git a/test/index.js b/test/index.js index 78c199fdf..d36749a2c 100644 --- a/test/index.js +++ b/test/index.js @@ -11,12 +11,12 @@ import 'babel-polyfill' * Tests. */ -import './plugins' import './rendering' import './schema' import './serializers' import './transforms' import './behavior' +import './plugins' /** * Reset Slate's internal state before each text. diff --git a/test/plugins/index.js b/test/plugins/index.js index b69c2c783..ee477476a 100644 --- a/test/plugins/index.js +++ b/test/plugins/index.js @@ -12,7 +12,7 @@ import { resolve } from 'path' * Tests. */ -describe.only('plugins', () => { +describe('plugins', () => { const tests = fs.readdirSync(resolve(__dirname, './fixtures')) for (const test of tests) {