Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance by caching model node index and offset in Node and NodeList #17296

Merged
merged 26 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1b3ac12
Tests: Introduce automated manual test sample for performance testing.
scofalik Oct 2, 2024
362ab43
Revert correct default values.
scofalik Oct 2, 2024
3068276
Slight code style change.
scofalik Oct 2, 2024
ffa8952
Introduced `init-single.html` manual test to test just one run of one…
scofalik Oct 2, 2024
1bd2430
Change structure in manual tests so that utils and data JS files are …
scofalik Oct 2, 2024
6ff5c40
Fixed lint problems.
scofalik Oct 3, 2024
45e80bb
Added three new data sets. Changed how `init-single` gets the initial…
scofalik Oct 4, 2024
556b767
Allow to use an array with data sets in automated manual performance …
scofalik Oct 9, 2024
fca0a8d
Fix time reporting for `init-single.js` test.
scofalik Oct 14, 2024
2d36e20
Fixed lint errors.
scofalik Oct 16, 2024
35313d9
Merge branch 'master' into cc/performance-research-base
oleq Oct 16, 2024
e7560b1
Rename `_data/generated` to `_data/data-sets`. Added `wiki.js` test c…
scofalik Oct 21, 2024
2413974
Fixed in data samples index file.
scofalik Oct 21, 2024
a9d77d6
Fixes in data samples index file.
scofalik Oct 21, 2024
17933ae
Another fix.
scofalik Oct 21, 2024
c5b03a7
Disable lint for one of the data sets.
scofalik Oct 22, 2024
3bad247
Updated manual tests descriptions.
scofalik Oct 22, 2024
540d6b6
Feature (engine): Introduced `getChildAtOffset()` method for `model.E…
scofalik Oct 15, 2024
5ae57e2
Feature (engine): Introduced `Position#isValid()` to check whether th…
scofalik Oct 16, 2024
7d24d6a
Fix lint error.
scofalik Oct 22, 2024
97bd789
Apply suggestions from code review
scofalik Oct 22, 2024
2e7cf35
Add missing docs.
scofalik Oct 22, 2024
490a9aa
Improve `NodeList#_makeOffsetsArray()` implementation.
scofalik Oct 22, 2024
7d4284d
Move `_makeOffsetsArray()` method to a util function.
scofalik Oct 22, 2024
0c7039f
Docs fixed typo.
scofalik Oct 22, 2024
055753f
Merge branch 'master' into cc/performance-offsets
Dumluregn Oct 23, 2024
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
3 changes: 2 additions & 1 deletion packages/ckeditor5-engine/src/model/document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,8 @@ export default class Document extends /* #__PURE__ */ EmitterMixin() {
* @returns `true` if `range` is valid, `false` otherwise.
*/
public _validateSelectionRange( range: Range ): boolean {
return validateTextNodePosition( range.start ) && validateTextNodePosition( range.end );
return range.start.isValid() && range.end.isValid() &&
validateTextNodePosition( range.start ) && validateTextNodePosition( range.end );
}

/**
Expand Down
16 changes: 13 additions & 3 deletions packages/ckeditor5-engine/src/model/documentfragment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,23 @@ export default class DocumentFragment extends TypeCheckable implements Iterable<
/**
* Gets the child at the given index. Returns `null` if incorrect index was passed.
*
* @param index Index of child.
* @param index Index in this document fragment.
* @returns Child node.
*/
public getChild( index: number ): Node | null {
return this._children.getNode( index );
}

/**
* Gets the child at the given offset. Returns `null` if incorrect index was passed.
*
* @param offset Offset in this document fragment.
* @returns Child node.
*/
public getChildAtOffset( offset: number ): Node | null {
return this._children.getNodeAtOffset( offset );
}

/**
* Returns an iterator that iterates over all of this document fragment's children.
*/
Expand Down Expand Up @@ -208,8 +218,8 @@ export default class DocumentFragment extends TypeCheckable implements Iterable<
// eslint-disable-next-line @typescript-eslint/no-this-alias, consistent-this
let node: Node | DocumentFragment = this;

for ( const index of relativePath ) {
node = ( node as Element | DocumentFragment ).getChild( ( node as Element | DocumentFragment ).offsetToIndex( index ) )!;
for ( const offset of relativePath ) {
node = ( node as Element | DocumentFragment ).getChildAtOffset( offset )!;
}

return node;
Expand Down
19 changes: 16 additions & 3 deletions packages/ckeditor5-engine/src/model/element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,25 @@ export default class Element extends Node {
}

/**
* Gets the child at the given index.
* Gets the child at the given index. Returns `null` if incorrect index was passed.
*
* @param index Index in this element.
* @returns Child node.
*/
public getChild( index: number ): Node | null {
return this._children.getNode( index );
}

/**
* Gets the child at the given offset. Returns `null` if incorrect index was passed.
*
* @param offset Offset in this element.
* @returns Child node.
*/
public getChildAtOffset( offset: number ): Node | null {
return this._children.getNodeAtOffset( offset );
}

/**
* Returns an iterator that iterates over all of this element's children.
*/
Expand Down Expand Up @@ -153,8 +166,8 @@ export default class Element extends Node {
// eslint-disable-next-line @typescript-eslint/no-this-alias, consistent-this
let node: Node = this;

for ( const index of relativePath ) {
node = ( node as Element ).getChild( ( node as Element ).offsetToIndex( index ) )!;
for ( const offset of relativePath ) {
node = ( node as Element ).getChildAtOffset( offset )!;
}

return node;
Expand Down
68 changes: 26 additions & 42 deletions packages/ckeditor5-engine/src/model/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ export default abstract class Node extends TypeCheckable {
*/
private _attrs: Map<string, unknown>;

/**
* Index of this node in its parent or `null` if the node has no parent.
*
* @internal
*/
public _index: number | null = null;

/**
* Offset at which this node starts in its parent or `null` if the node has no parent.
*
* @internal
*/
public _startOffset: number | null = null;

/**
* Creates a model node.
*
Expand All @@ -85,66 +99,42 @@ export default abstract class Node extends TypeCheckable {

/**
* Index of this node in its parent or `null` if the node has no parent.
*
* Accessing this property throws an error if this node's parent element does not contain it.
* This means that model tree got broken.
*/
public get index(): number | null {
let pos;

if ( !this.parent ) {
return null;
}

if ( ( pos = this.parent.getChildIndex( this ) ) === null ) {
throw new CKEditorError( 'model-node-not-found-in-parent', this );
}

return pos;
return this._index;
}

/**
* Offset at which this node starts in its parent. It is equal to the sum of {@link #offsetSize offsetSize}
* of all its previous siblings. Equals to `null` if node has no parent.
*
* Accessing this property throws an error if this node's parent element does not contain it.
* This means that model tree got broken.
*/
public get startOffset(): number | null {
let pos;

if ( !this.parent ) {
return null;
}

if ( ( pos = this.parent.getChildStartOffset( this ) ) === null ) {
throw new CKEditorError( 'model-node-not-found-in-parent', this );
}

return pos;
return this._startOffset;
}

/**
* Offset size of this node. Represents how much "offset space" is occupied by the node in it's parent.
* It is important for {@link module:engine/model/position~Position position}. When node has `offsetSize` greater than `1`, position
* can be placed between that node start and end. `offsetSize` greater than `1` is for nodes that represents more
* than one entity, i.e. {@link module:engine/model/text~Text text node}.
* Offset size of this node.
*
* Represents how much "offset space" is occupied by the node in its parent. It is important for
* {@link module:engine/model/position~Position position}. When node has `offsetSize` greater than `1`, position can be placed between
* that node start and end. `offsetSize` greater than `1` is for nodes that represents more than one entity, i.e.
* a {@link module:engine/model/text~Text text node}.
*/
public get offsetSize(): number {
return 1;
}

/**
* Offset at which this node ends in it's parent. It is equal to the sum of this node's
* Offset at which this node ends in its parent. It is equal to the sum of this node's
* {@link module:engine/model/node~Node#startOffset start offset} and {@link #offsetSize offset size}.
* Equals to `null` if the node has no parent.
*/
public get endOffset(): number | null {
if ( !this.parent ) {
if ( this.startOffset === null ) {
return null;
}

return this.startOffset! + this.offsetSize;
return this.startOffset + this.offsetSize;
}

/**
Expand Down Expand Up @@ -387,7 +377,7 @@ export default abstract class Node extends TypeCheckable {
}

/**
* Removes this node from it's parent.
* Removes this node from its parent.
*
* @internal
* @see module:engine/model/writer~Writer#remove
Expand Down Expand Up @@ -448,12 +438,6 @@ Node.prototype.is = function( type: string ): boolean {
return type === 'node' || type === 'model:node';
};

/**
* The node's parent does not contain this node.
*
* @error model-node-not-found-in-parent
*/

/**
* Node's attributes. See {@link module:utils/tomap~toMap} for a list of accepted values.
*/
Expand Down
Loading