Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Commit 08e8294

Browse files
ReinmarReinmar
authored
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.
2 parents bfc6c88 + ccb6985 commit 08e8294

File tree

3 files changed

+146
-29
lines changed

3 files changed

+146
-29
lines changed

‎src/model/position.js

Lines changed: 84 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ export default class Position {
218218
* @type {module:engine/model/text~Text|null}
219219
*/
220220
get textNode() {
221-
return getTextNode( this, this.parent );
221+
return getTextNodeAtPosition( this, this.parent );
222222
}
223223

224224
/**
@@ -228,16 +228,10 @@ export default class Position {
228228
* @type {module:engine/model/node~Node|null}
229229
*/
230230
get nodeAfter() {
231-
// Cache parent and reuse for performance reasons. This also means that we cannot use the #index property.
232-
// See #6579.
231+
// Cache the parent and reuse for performance reasons. See #6579 and #6582.
233232
const parent = this.parent;
234-
const textNode = getTextNode( this, parent );
235233

236-
if ( textNode !== null ) {
237-
return null;
238-
}
239-
240-
return parent.getChild( parent.offsetToIndex( this.offset ) );
234+
return getNodeAfterPosition( this, parent, getTextNodeAtPosition( this, parent ) );
241235
}
242236

243237
/**
@@ -247,16 +241,10 @@ export default class Position {
247241
* @type {module:engine/model/node~Node|null}
248242
*/
249243
get nodeBefore() {
250-
// Cache parent and reuse for performance reasons. This also means that we cannot use the #index property.
251-
// See #6579.
244+
// Cache the parent and reuse for performance reasons. See #6579 and #6582.
252245
const parent = this.parent;
253-
const textNode = getTextNode( this, parent );
254246

255-
if ( textNode !== null ) {
256-
return null;
257-
}
258-
259-
return parent.getChild( parent.offsetToIndex( this.offset ) - 1 );
247+
return getNodeBeforePosition( this, parent, getTextNodeAtPosition( this, parent ) );
260248
}
261249

262250
/**
@@ -1078,10 +1066,28 @@ export default class Position {
10781066
* @typedef {String} module:engine/model/position~PositionStickiness
10791067
*/
10801068

1081-
// Helper function used to inline text node access by using a cached parent.
1082-
// Reduces the access to the Position#parent property 3 times (in total, when taken into account what #nodeAfter and #nodeBefore do).
1083-
// See #6579.
1084-
function getTextNode( position, positionParent ) {
1069+
/**
1070+
* Returns a text node at the given position.
1071+
*
1072+
* This is a helper function optimized to reuse the position parent instance for performance reasons.
1073+
*
1074+
* Normally, you should use {@link module:engine/model/position~Position#textNode `Position#textNode`}.
1075+
* If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`}
1076+
* check if your algorithm does not access it multiple times (which can happen directly or indirectly via other position properties).
1077+
*
1078+
* See https://github.com/ckeditor/ckeditor5/issues/6579.
1079+
*
1080+
* See also:
1081+
*
1082+
* * {@link module:engine/model/position~getNodeAfterPosition}
1083+
* * {@link module:engine/model/position~getNodeBeforePosition}
1084+
*
1085+
* @param {module:engine/model/position~Position} position
1086+
* @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
1087+
* given position.
1088+
* @returns {module:engine/model/text~Text|null}
1089+
*/
1090+
export function getTextNodeAtPosition( position, positionParent ) {
10851091
const node = positionParent.getChild( positionParent.offsetToIndex( position.offset ) );
10861092

10871093
if ( node && node.is( 'text' ) && node.startOffset < position.offset ) {
@@ -1090,3 +1096,60 @@ function getTextNode( position, positionParent ) {
10901096

10911097
return null;
10921098
}
1099+
1100+
/**
1101+
* Returns the node after the given position.
1102+
*
1103+
* This is a helper function optimized to reuse the position parent instance and the calculation of the text node at the
1104+
* specific position for performance reasons.
1105+
*
1106+
* Normally, you should use {@link module:engine/model/position~Position#nodeAfter `Position#nodeAfter`}.
1107+
* If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`} and/or
1108+
* {@link module:engine/model/position~Position#textNode `Position#textNode`}
1109+
* check if your algorithm does not access those properties multiple times
1110+
* (which can happen directly or indirectly via other position properties).
1111+
*
1112+
* See https://github.com/ckeditor/ckeditor5/issues/6579 and https://github.com/ckeditor/ckeditor5/issues/6582.
1113+
*
1114+
* See also:
1115+
*
1116+
* * {@link module:engine/model/position~getTextNodeAtPosition}
1117+
* * {@link module:engine/model/position~getNodeBeforePosition}
1118+
*
1119+
* @param {module:engine/model/position~Position} position
1120+
* @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
1121+
* given position.
1122+
* @param {module:engine/model/text~Text|null} textNode Text node at the given position.
1123+
* @returns {module:engine/model/node~Node|null}
1124+
*/
1125+
export function getNodeAfterPosition( position, positionParent, textNode ) {
1126+
if ( textNode !== null ) {
1127+
return null;
1128+
}
1129+
1130+
return positionParent.getChild( positionParent.offsetToIndex( position.offset ) );
1131+
}
1132+
1133+
/**
1134+
* Returns the node before the given position.
1135+
*
1136+
* Refer to {@link module:engine/model/position~getNodeBeforePosition} for documentation on when to use this util method.
1137+
*
1138+
* See also:
1139+
*
1140+
* * {@link module:engine/model/position~getTextNodeAtPosition}
1141+
* * {@link module:engine/model/position~getNodeAfterPosition}
1142+
*
1143+
* @param {module:engine/model/position~Position} position
1144+
* @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the
1145+
* given position.
1146+
* @param {module:engine/model/text~Text|null} textNode Text node at the given position.
1147+
* @returns {module:engine/model/node~Node|null}
1148+
*/
1149+
export function getNodeBeforePosition( position, positionParent, textNode ) {
1150+
if ( textNode !== null ) {
1151+
return null;
1152+
}
1153+
1154+
return positionParent.getChild( positionParent.offsetToIndex( position.offset ) - 1 );
1155+
}

‎src/model/treewalker.js

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,12 @@
1010
import Text from './text';
1111
import TextProxy from './textproxy';
1212
import Element from './element';
13-
import Position from './position';
13+
import {
14+
default as Position,
15+
getTextNodeAtPosition,
16+
getNodeAfterPosition,
17+
getNodeBeforePosition
18+
} from './position';
1419
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
1520

1621
/**
@@ -225,7 +230,11 @@ export default class TreeWalker {
225230
return { done: true };
226231
}
227232

228-
const node = position.textNode ? position.textNode : position.nodeAfter;
233+
// Get node just after the current position.
234+
// Use a highly optimized version instead of checking the text node first and then getting the node after. See #6582.
235+
const positionParent = position.parent;
236+
const textNodeAtPosition = getTextNodeAtPosition( position, positionParent );
237+
const node = textNodeAtPosition ? textNodeAtPosition : getNodeAfterPosition( position, positionParent, textNodeAtPosition );
229238

230239
if ( node instanceof Element ) {
231240
if ( !this.shallow ) {
@@ -299,8 +308,11 @@ export default class TreeWalker {
299308
return { done: true };
300309
}
301310

302-
// Get node just before current position
303-
const node = position.textNode ? position.textNode : position.nodeBefore;
311+
// Get node just before the current position.
312+
// Use a highly optimized version instead of checking the text node first and then getting the node before. See #6582.
313+
const positionParent = position.parent;
314+
const textNodeAtPosition = getTextNodeAtPosition( position, positionParent );
315+
const node = textNodeAtPosition ? textNodeAtPosition : getNodeBeforePosition( position, positionParent, textNodeAtPosition );
304316

305317
if ( node instanceof Element ) {
306318
position.offset--;

‎tests/model/position.js

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ import DocumentFragment from '../../src/model/documentfragment';
88
import Element from '../../src/model/element';
99
import Text from '../../src/model/text';
1010
import TextProxy from '../../src/model/textproxy';
11-
import Position from '../../src/model/position';
11+
import {
12+
default as Position,
13+
getTextNodeAtPosition,
14+
getNodeAfterPosition,
15+
getNodeBeforePosition
16+
} from '../../src/model/position';
1217
import Range from '../../src/model/range';
1318
import MarkerOperation from '../../src/model/operation/markeroperation';
1419
import AttributeOperation from '../../src/model/operation/attributeoperation';
@@ -485,10 +490,12 @@ describe( 'Position', () => {
485490
} );
486491
} );
487492

488-
it( 'should have proper parent path', () => {
489-
const position = new Position( root, [ 1, 2, 3 ] );
493+
describe( 'getParentPath()', () => {
494+
it( 'should have proper parent path', () => {
495+
const position = new Position( root, [ 1, 2, 3 ] );
490496

491-
expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] );
497+
expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] );
498+
} );
492499
} );
493500

494501
describe( 'isBefore()', () => {
@@ -1275,4 +1282,39 @@ describe( 'Position', () => {
12751282
expect( positionB.getCommonAncestor( positionA ) ).to.equal( lca );
12761283
}
12771284
} );
1285+
1286+
describe( 'getTextNodeAtPosition() util', () => {
1287+
it( 'returns a text node at the given position', () => {
1288+
const position = new Position( root, [ 1, 0, 1 ] );
1289+
const positionParent = position.parent;
1290+
1291+
expect( getTextNodeAtPosition( position, positionParent ) ).to.equal( foz );
1292+
} );
1293+
1294+
// This util is covered with tests by Position#textNode tests.
1295+
} );
1296+
1297+
describe( 'getNodeAfterPosition() util', () => {
1298+
it( 'returns a node after the position', () => {
1299+
const position = new Position( root, [ 1, 0 ] );
1300+
const positionParent = position.parent;
1301+
const textNode = getTextNodeAtPosition( position, positionParent );
1302+
1303+
expect( getNodeAfterPosition( position, positionParent, textNode ) ).to.equal( li1 );
1304+
} );
1305+
1306+
// This util is covered with tests by Position#nodeAfter tests.
1307+
} );
1308+
1309+
describe( 'getNodeBeforePosition() util', () => {
1310+
it( 'returns a node before the position', () => {
1311+
const position = new Position( root, [ 1, 1 ] );
1312+
const positionParent = position.parent;
1313+
const textNode = getTextNodeAtPosition( position, positionParent );
1314+
1315+
expect( getNodeBeforePosition( position, positionParent, textNode ) ).to.equal( li1 );
1316+
} );
1317+
1318+
// This util is covered with tests by Position#nodeBefore tests.
1319+
} );
12781320
} );

0 commit comments

Comments
 (0)