Merge pull request #1839 from ckeditor/i/6582

Other: Improved performance of `TreeWalker` by up to 40%. This optimization affects common tasks such as loading editor data. Closes ckeditor/ckeditor5#6582.
ckeditor · Apr 14, 2020 · 08e8294 · 08e8294
2 parents bfc6c88 + ccb6985
commit 08e8294
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 29 deletions.
diff --git a/src/model/position.js b/src/model/position.js
@@ -218,7 +218,7 @@ export default class Position {
 	 * @type {module:engine/model/text~Text|null}
 	 */
 	get textNode() {
-		return getTextNode( this, this.parent );
+		return getTextNodeAtPosition( this, this.parent );
 	}
 
 	/**
@@ -228,16 +228,10 @@ export default class Position {
 	 * @type {module:engine/model/node~Node|null}
 	 */
 	get nodeAfter() {
-		// Cache parent and reuse for performance reasons. This also means that we cannot use the #index property.
-		// See #6579.
+		// Cache the parent and reuse for performance reasons. See #6579 and #6582.
 		const parent = this.parent;
-		const textNode = getTextNode( this, parent );
 
-		if ( textNode !== null ) {
-			return null;
-		}
-
-		return parent.getChild( parent.offsetToIndex( this.offset ) );
+		return getNodeAfterPosition( this, parent, getTextNodeAtPosition( this, parent ) );
 	}
 
 	/**
@@ -247,16 +241,10 @@ export default class Position {
 	 * @type {module:engine/model/node~Node|null}
 	 */
 	get nodeBefore() {
-		// Cache parent and reuse for performance reasons. This also means that we cannot use the #index property.
-		// See #6579.
+		// Cache the parent and reuse for performance reasons. See #6579 and #6582.
 		const parent = this.parent;
-		const textNode = getTextNode( this, parent );
 
-		if ( textNode !== null ) {
-			return null;
-		}
-
-		return parent.getChild( parent.offsetToIndex( this.offset ) - 1 );
+		return getNodeBeforePosition( this, parent, getTextNodeAtPosition( this, parent ) );
 	}
 
 	/**
@@ -1078,10 +1066,28 @@ export default class Position {
  * @typedef {String} module:engine/model/position~PositionStickiness
  */
 
-// Helper function used to inline text node access by using a cached parent.
-// Reduces the access to the Position#parent property 3 times (in total, when taken into account what #nodeAfter and #nodeBefore do).
-// See #6579.
-function getTextNode( position, positionParent ) {
+/**
+ * Returns a text node at the given position.
+ *
+ * This is a helper function optimized to reuse the position parent instance for performance reasons.
+ *
+ * Normally, you should use {@link module:engine/model/position~Position#textNode `Position#textNode`}.
+ * If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`}
+ * check if your algorithm does not access it multiple times (which can happen directly or indirectly via other position properties).
+ *
+ * See https://github.com/ckeditor/ckeditor5/issues/6579.
+ *
+ * See also:
+ *
+ * * {@link module:engine/model/position~getNodeAfterPosition}
+ * * {@link module:engine/model/position~getNodeBeforePosition}
+ *
+ * @param {module:engine/model/position~Position} position
+ * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
+ * given position.
+ * @returns {module:engine/model/text~Text|null}
+ */
+export function getTextNodeAtPosition( position, positionParent ) {
 	const node = positionParent.getChild( positionParent.offsetToIndex( position.offset ) );
 
 	if ( node && node.is( 'text' ) && node.startOffset < position.offset ) {
@@ -1090,3 +1096,60 @@ function getTextNode( position, positionParent ) {
 
 	return null;
 }
+
+/**
+ * Returns the node after the given position.
+ *
+ * This is a helper function optimized to reuse the position parent instance and the calculation of the text node at the
+ * specific position for performance reasons.
+ *
+ * Normally, you should use {@link module:engine/model/position~Position#nodeAfter `Position#nodeAfter`}.
+ * If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`} and/or
+ * {@link module:engine/model/position~Position#textNode `Position#textNode`}
+ * check if your algorithm does not access those properties multiple times
+ * (which can happen directly or indirectly via other position properties).
+ *
+ * See https://github.com/ckeditor/ckeditor5/issues/6579 and https://github.com/ckeditor/ckeditor5/issues/6582.
+ *
+ * See also:
+ *
+ * * {@link module:engine/model/position~getTextNodeAtPosition}
+ * * {@link module:engine/model/position~getNodeBeforePosition}
+ *
+ * @param {module:engine/model/position~Position} position
+ * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
+ * given position.
+ * @param {module:engine/model/text~Text|null} textNode Text node at the given position.
+ * @returns {module:engine/model/node~Node|null}
+ */
+export function getNodeAfterPosition( position, positionParent, textNode ) {
+	if ( textNode !== null ) {
+		return null;
+	}
+
+	return positionParent.getChild( positionParent.offsetToIndex( position.offset ) );
+}
+
+/**
+ * Returns the node before the given position.
+ *
+ * Refer to {@link module:engine/model/position~getNodeBeforePosition} for documentation on when to use this util method.
+ *
+ * See also:
+ *
+ * * {@link module:engine/model/position~getTextNodeAtPosition}
+ * * {@link module:engine/model/position~getNodeAfterPosition}
+ *
+ * @param {module:engine/model/position~Position} position
+ * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
+ * given position.
+ * @param {module:engine/model/text~Text|null} textNode Text node at the given position.
+ * @returns {module:engine/model/node~Node|null}
+ */
+export function getNodeBeforePosition( position, positionParent, textNode ) {
+	if ( textNode !== null ) {
+		return null;
+	}
+
+	return positionParent.getChild( positionParent.offsetToIndex( position.offset ) - 1 );
+}
diff --git a/src/model/treewalker.js b/src/model/treewalker.js
@@ -10,7 +10,12 @@
 import Text from './text';
 import TextProxy from './textproxy';
 import Element from './element';
-import Position from './position';
+import {
+	default as Position,
+	getTextNodeAtPosition,
+	getNodeAfterPosition,
+	getNodeBeforePosition
+} from './position';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
@@ -225,7 +230,11 @@ export default class TreeWalker {
 			return { done: true };
 		}
 
-		const node = position.textNode ? position.textNode : position.nodeAfter;
+		// Get node just after the current position.
+		// Use a highly optimized version instead of checking the text node first and then getting the node after. See #6582.
+		const positionParent = position.parent;
+		const textNodeAtPosition = getTextNodeAtPosition( position, positionParent );
+		const node = textNodeAtPosition ? textNodeAtPosition : getNodeAfterPosition( position, positionParent, textNodeAtPosition );
 
 		if ( node instanceof Element ) {
 			if ( !this.shallow ) {
@@ -299,8 +308,11 @@ export default class TreeWalker {
 			return { done: true };
 		}
 
-		// Get node just before current position
-		const node = position.textNode ? position.textNode : position.nodeBefore;
+		// Get node just before the current position.
+		// Use a highly optimized version instead of checking the text node first and then getting the node before. See #6582.
+		const positionParent = position.parent;
+		const textNodeAtPosition = getTextNodeAtPosition( position, positionParent );
+		const node = textNodeAtPosition ? textNodeAtPosition : getNodeBeforePosition( position, positionParent, textNodeAtPosition );
 
 		if ( node instanceof Element ) {
 			position.offset--;

diff --git a/tests/model/position.js b/tests/model/position.js
@@ -8,7 +8,12 @@ import DocumentFragment from '../../src/model/documentfragment';
 import Element from '../../src/model/element';
 import Text from '../../src/model/text';
 import TextProxy from '../../src/model/textproxy';
-import Position from '../../src/model/position';
+import {
+	default as Position,
+	getTextNodeAtPosition,
+	getNodeAfterPosition,
+	getNodeBeforePosition
+} from '../../src/model/position';
 import Range from '../../src/model/range';
 import MarkerOperation from '../../src/model/operation/markeroperation';
 import AttributeOperation from '../../src/model/operation/attributeoperation';
@@ -485,10 +490,12 @@ describe( 'Position', () => {
 		} );
 	} );
 
-	it( 'should have proper parent path', () => {
-		const position = new Position( root, [ 1, 2, 3 ] );
+	describe( 'getParentPath()', () => {
+		it( 'should have proper parent path', () => {
+			const position = new Position( root, [ 1, 2, 3 ] );
 
-		expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] );
+			expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] );
+		} );
 	} );
 
 	describe( 'isBefore()', () => {
@@ -1275,4 +1282,39 @@ describe( 'Position', () => {
 			expect( positionB.getCommonAncestor( positionA ) ).to.equal( lca );
 		}
 	} );
+
+	describe( 'getTextNodeAtPosition() util', () => {
+		it( 'returns a text node at the given position', () => {
+			const position = new Position( root, [ 1, 0, 1 ] );
+			const positionParent = position.parent;
+
+			expect( getTextNodeAtPosition( position, positionParent ) ).to.equal( foz );
+		} );
+
+		// This util is covered with tests by Position#textNode tests.
+	} );
+
+	describe( 'getNodeAfterPosition() util', () => {
+		it( 'returns a node after the position', () => {
+			const position = new Position( root, [ 1, 0 ] );
+			const positionParent = position.parent;
+			const textNode = getTextNodeAtPosition( position, positionParent );
+
+			expect( getNodeAfterPosition( position, positionParent, textNode ) ).to.equal( li1 );
+		} );
+
+		// This util is covered with tests by Position#nodeAfter tests.
+	} );
+
+	describe( 'getNodeBeforePosition() util', () => {
+		it( 'returns a node before the position', () => {
+			const position = new Position( root, [ 1, 1 ] );
+			const positionParent = position.parent;
+			const textNode = getTextNodeAtPosition( position, positionParent );
+
+			expect( getNodeBeforePosition( position, positionParent, textNode ) ).to.equal( li1 );
+		} );
+
+		// This util is covered with tests by Position#nodeBefore tests.
+	} );
 } );