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

I/6528: Faster paths concatenation #1835

Merged
merged 13 commits into from
Apr 14, 2020
Merged
15 changes: 9 additions & 6 deletions src/model/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import TreeWalker from './treewalker';
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import { last } from 'lodash-es';

// To check if component is loaded more than once.
import '@ckeditor/ckeditor5-utils/src/version';
Expand Down Expand Up @@ -80,9 +79,13 @@ export default class Position {
);
}

// Normalize the root and path (if element was passed).
path = root.getPath().concat( path );
root = root.root;
// Normalize the root and path when element (not root) is passed.
if ( root.is( 'rootElement' ) ) {
path = path.slice();
} else {
path = [ ...root.getPath(), ...path ];
root = root.root;
}

/**
* Root of the position path.
Expand Down Expand Up @@ -140,7 +143,7 @@ export default class Position {
* @type {Number}
*/
get offset() {
return last( this.path );
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? I thought myself that last() is probably slow, but I didn't see any change in results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a need here for using an external library which involves loading an external library for this. Also for other path operations, we do not use libs for simple tasks in Position elsewhere.

The last helper does:

function last(array) {
  var length = array == null ? 0 : array.length;
  return length ? array[length - 1] : undefined;
}

and we should never have Position without this.path.

Calling external method will be slower. It is arguable if this is an important gain. Here I get 195.7ms (1.1%) down to 2.3 ms (0.0%). In a total of ~20s (long semantic + undo). So it is arguable if this change is necessary - I'm for this change.

Current:

Current with reverting to last.
Selection_307

TL;DR: Let's keep it - using last() makes no sense.

return this.path[ this.path.length - 1 ];
}

/**
Expand Down Expand Up @@ -857,7 +860,7 @@ export default class Position {

// Then, add the rest of the path.
// If this position is at the same level as `from` position nothing will get added.
combined.path = combined.path.concat( this.path.slice( i + 1 ) );
combined.path = [ ...combined.path, ...this.path.slice( i + 1 ) ];

return combined;
}
Expand Down
7 changes: 7 additions & 0 deletions tests/manual/performance/position.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<div id="test-controls">
<button id="run">Run</button>
</div>

<hr>

<div id="output"></div>
161 changes: 161 additions & 0 deletions tests/manual/performance/position.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/**
* @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* globals window, document, setTimeout */

document.getElementById( 'run' ).addEventListener( 'click', () => {
log( 'Running tests...' );

setTimeout( async () => {
await runTest( 'concat', testConcat );
await runTest( 'spread operator', testSpread );
await runTest( 'for-loop', testForLoop );
await runTest( 'ultra-loop', testUltraForLoop );

log( 'done' );
} );
} );

window.cache = [];

const output = document.getElementById( 'output' );

function log( line ) {
const paragraphElement = document.createElement( 'p' );
paragraphElement.innerText = line;
output.appendChild( paragraphElement );
}

function runTest( name, callback ) {
return new Promise( resolve => {
const start = new Date();

const repetitions = 10000000;

const root = {
root: 'foo',
path: [ 0 ]
};
const path = [ 0, 2 ];

for ( let i = 0; i < repetitions; i++ ) {
const newPath = callback( root, path );
window.cache.push( newPath.length );
}

const end = new Date();

log( ` > ${ name } took ${ end - start }ms` );

setTimeout( () => {
resolve();
}, 50 );
} );
}

class PositionConcat {
constructor( root, path, stickiness = 'left' ) {
if ( !( path instanceof Array ) || path.length === 0 ) {
throw new Error( 'model-position-path-incorrect-format' );
}

path = root.path.concat( path );
root = root.root;

this.root = root;
this.path = path;
this.stickiness = stickiness;
}
}

class PositionSpread {
constructor( root, path, stickiness = 'left' ) {
if ( !( path instanceof Array ) || path.length === 0 ) {
throw new Error( 'model-position-path-incorrect-format' );
}

path = [ ...root.path, ...path ];
root = root.root;

this.root = root;
this.path = path;
this.stickiness = stickiness;
}
}

class PositionForLoop {
constructor( root, path, stickiness = 'left' ) {
if ( !( path instanceof Array ) || path.length === 0 ) {
throw new Error( 'model-position-path-incorrect-format' );
}

path = forLoop( root.path, path );
root = root.root;

this.root = root;
this.path = path;
this.stickiness = stickiness;
}
}

class PositionUltraForLoop {
constructor( root, path, stickiness = 'left' ) {
if ( !( path instanceof Array ) || path.length === 0 ) {
throw new Error( 'model-position-path-incorrect-format' );
}

path = ultraForLoop( root.path, path );
root = root.root;

this.root = root;
this.path = path;
this.stickiness = stickiness;
}
}

function testConcat( root, path ) {
return new PositionConcat( root, path );
}

function testSpread( root, path ) {
return new PositionSpread( root, path );
}

function testForLoop( root, path ) {
return new PositionForLoop( root, path );
}

function testUltraForLoop( root, path ) {
return new PositionUltraForLoop( root, path );
}

function forLoop( rootPath, path ) {
const newPath = [];

for ( let i = 0; i < rootPath.length; i++ ) {
newPath.push( rootPath[ i ] );
}

for ( let i = 0; i < path.length; i++ ) {
newPath.push( path[ i ] );
}

return newPath;
}

function ultraForLoop( rootPath, path ) {
const fullLength = rootPath.length + path.length;
const newPath = new Array( fullLength );

for ( let i = 0; i < rootPath.length; i++ ) {
newPath[ i ] = rootPath[ i ];
}

for ( let i = 0; i < path.length; i++ ) {
newPath[ rootPath.length + i ] = path[ i ];
}

return newPath;
}
7 changes: 7 additions & 0 deletions tests/manual/performance/position.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Performance: pasting data

1. Begin performance recording in devtools.
1. Click a button for test.
1. Stop performance recording.

Note that times reported in web page are total time for a test function (including other operations). In order to verify which methods is fastest always look in on the "total time" of each Position constructor stubs.