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 all 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
15 changes: 9 additions & 6 deletions src/model/position.js
View file
Open in desktop
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