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

I/6528: Faster paths concatenation #1835

Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4386a68
Use spread operator for concatenating paths in Position constructor.
jodator Apr 1, 2020
a5e9280
Use for loops to concatenate paths in model/Position.
jodator Apr 1, 2020
abf6d1f
Add docs to helper concatenatePaths() helper method.
jodator Apr 2, 2020
75a53ec
Do not use library method in Position#offset.
jodator Apr 3, 2020
e49c55c
Add simple manual test for Position's path concatenation.
jodator Apr 6, 2020
eae2b48
Use position class to test performance.
jodator Apr 9, 2020
f8e5c8f
Use separate position classes.
jodator Apr 9, 2020
5708ba2
Fix test performance.
jodator Apr 9, 2020
184cb3d
Merge branch 'master' into i/6528
jodator Apr 9, 2020
dada66d
Add note about times in tests.
jodator Apr 9, 2020
5485dbc
Merge branch 'master' into i/6528
Reinmar Apr 13, 2020
3c7ea63
The root is passed most commonly and it has an empty path, so we can …
Reinmar Apr 13, 2020
e63694a
Removed a test that's unnecessary now.
Reinmar Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/model/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;

/**
Expand Down Expand Up @@ -141,7 +140,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 @@ -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;
}
Expand Down Expand Up @@ -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 = [];
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

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 Array.concat() as this method do too many things AFAICS (must check each parameter from args if it is an array, etc).

Responding also to spread operator. It is slower. I'll post the results below. Also, I'll add more comments in the code so one would remember why for and not [ ...root, ...path ].

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@jodator jodator Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So the results are

1. Proposed solution

2. Spread operator:

3. Ultra optimized array creation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Firefox (using medium sample because FF has short buffer and it way to slow there):

1. Proposed

2. Spread operator

It looks like there is no difference there but number of samples is different so it looks like the performance buffer was overflown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}