-
Notifications
You must be signed in to change notification settings - Fork 40
I/6528: Faster paths concatenation #1835
Changes from 10 commits
4386a68
a5e9280
abf6d1f
75a53ec
e49c55c
eae2b48
f8e5c8f
5708ba2
184cb3d
dada66d
5485dbc
3c7ea63
e63694a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,6 @@ import TreeWalker from './treewalker'; | |
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays'; | ||
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; | ||
import Text from './text'; | ||
import { last } from 'lodash-es'; | ||
|
||
// To check if component is loaded more than once. | ||
import '@ckeditor/ckeditor5-utils/src/version'; | ||
|
@@ -81,8 +80,8 @@ export default class Position { | |
); | ||
} | ||
|
||
// Normalize the root and path (if element was passed). | ||
path = root.getPath().concat( path ); | ||
// Normalize the root and path when element (not root) is passed. | ||
path = concatenatePaths( root.getPath(), path ); | ||
root = root.root; | ||
|
||
/** | ||
|
@@ -141,7 +140,7 @@ export default class Position { | |
* @type {Number} | ||
*/ | ||
get offset() { | ||
return last( this.path ); | ||
return this.path[ this.path.length - 1 ]; | ||
} | ||
|
||
/** | ||
|
@@ -840,7 +839,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 = concatenatePaths( combined.path, this.path.slice( i + 1 ) ); | ||
|
||
return combined; | ||
} | ||
|
@@ -1057,3 +1056,22 @@ export default class Position { | |
* | ||
* @typedef {String} module:engine/model/position~PositionStickiness | ||
*/ | ||
|
||
// This helper method concatenate two arrays more efficiently than Array.concat(). See ckeditor/ckeditor5#6528. | ||
// | ||
// The problem with Array.concat() is that it is an overloaded method that can concatenate various arguments, | ||
// like mixed data types with arrays (e.g. [ 0 ].concat( 1, 2, [3, 4])) thus it probably check each argument's types and more. | ||
// In Position's paths concatenation case there always be two arrays to merge so such general method is not needed. | ||
function concatenatePaths( rootPath, path ) { | ||
const newPath = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you check whether creating new array with the desired length (`new Array(len)`) isn't better? Creating an empty array and pushing items to it seems extremely counterintuitive. I understand that right now we can see that it's faster, but this is this type of optimization that may suddenly become slower. Also, on which browsers did you test it? Due to this being counterintuitive, I'd like to be 100% sure that it doesn't deoptimize this on one of the browsers. It's probably one of the places where having a manual test comparing two scenarios might be helpful in verifying this in the future too. Because it's not said that the situation won't change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll check. The same thing was pointed out by @mlewand . I think that in this case, the change might be negligible as both arrays have length of max few values. This is potentially helpful when concatenating large arrays. The case that this might get slower will probably be a newer a case for Responding also to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, I'll check this with a manual test and post the results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So 1 & 3 are basically the same so we can choose either. |
||
|
||
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; | ||
} |
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> |
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; | ||
} |
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. |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:and we should never have
Position
withoutthis.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:
![](https://user-images.githubusercontent.com/247363/79196530-9285e580-7e30-11ea-9257-07e7be6ec231.png)
Current with reverting to
![Selection_307](https://user-images.githubusercontent.com/247363/79196602-a9c4d300-7e30-11ea-84cb-6e203d0152ed.png)
last
.TL;DR: Let's keep it - using
last()
makes no sense.