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.

Sign up for GitHub

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 of getSelectedBlocks #17582

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
+186 −1
Open

Improve performance of getSelectedBlocks #17582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bab66e5
Improve performance of `getSelectedBlocks`
filipsobol Dec 2, 2024
15ecaa4
Fix for failing lists tests
filipsobol Dec 3, 2024
cc29a9a
Simplify change
filipsobol Dec 3, 2024
b1e9b22
Add missing dot
filipsobol Dec 3, 2024
c759e5f
Ensure that fast path is only used for block elements
filipsobol Dec 3, 2024
5ae6c10
Add `jumpTo` to `TreeWalker`
filipsobol Dec 6, 2024
476e12d
Add tests and docs
filipsobol Dec 9, 2024
97250c3
Update `getSelectedBlocks` and `jumpTo`
filipsobol Dec 11, 2024
4883755
Fix typos
filipsobol Dec 11, 2024
dfff427
Avoid cloning the `Position` object, since it's not necessary
filipsobol Dec 11, 2024
05d5412
Apply suggestions from code review
filipsobol Dec 16, 2024
73f6109
Apply suggestions
filipsobol Dec 16, 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
12 changes: 11 additions & 1 deletion packages/ckeditor5-engine/src/model/selection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,12 +667,22 @@ export default class Selection extends /* #__PURE__ */ EmitterMixin( TypeCheckab
yield startBlock as any;
}

for ( const value of range.getWalker() ) {
const treewalker = range.getWalker();

for ( const value of treewalker ) {
const block = value.item;

if ( value.type == 'elementEnd' && isUnvisitedTopBlock( block as any, visited, range ) ) {
yield block as Element;
}
// If element is block, we can skip its children and jump to the end of it.
else if (
value.type == 'elementStart' &&
block.is( 'model:element' ) &&
block.root.document!.model.schema.isBlock( block )
) {
treewalker.jumpTo( Position._createAt( block, 'end' ) );
}
}

const endBlock = getParentBlock( range.end, visited );
Expand Down
26 changes: 26 additions & 0 deletions packages/ckeditor5-engine/src/model/treewalker.ts
View file
Open in desktop
filipsobol marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,32 @@ export default class TreeWalker implements Iterable<TreeWalkerValue> {
}
}

/**
* Moves tree walker {@link #position} to provided `position`. Tree walker will
* continue traversing from that position.
filipsobol marked this conversation as resolved.
Show resolved Hide resolved
*
* Note: in contrary to {@link ~TreeWalker#skip}, this method does not iterate over the nodes along the way.
* It simply sets the current tree walker position to a new one.
* From the performance standpoint, it is better to use {@link ~TreeWalker#jumpTo} rather than {@link ~TreeWalker#skip}.
*
* If the provided position is before the start boundary, the position will be
* set to the start boundary. If the provided position is after the end boundary,
* the position will be set to the end boundary.
* This is done to prevent the treewalker from traversing outside the boundaries.
*
* @param position Position to jump to.
*/
public jumpTo( position: Position ): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am worried that this method does not clone the position.

I know we should avoid cloning due to possible performance problems. However, it may lead to nasty bugs after using this function, when:

  • tree walker will keep on iterating and will change the position, that you may want to use at some later point,
  • or you will change that position offset and affect the tree walker.

So far, we don't have a guideline whether you (as integrator/developer) should always pass a "new" (cloned) position to API, or whether the functions will do it for you. We can add this note do API docs too, to help avoid creating unnecessary positions.

At the beginning of the performance initiative, I checked how how big impact has creating so many position clones on the overall performance. I was surprised how many positions we clone (a few hundreds thousands for some of the test samples). But I was also surprised that cutting it in half* didn't really have much impact. And in the profiler, I couldn't see that Position constructor takes a lot of time. Not sure now, though, as we optimized a lot of stuff, and the percentage gains may be more significant.

* - this change turned to be bugged anyway, but still, I could measure the performance change.

if ( this._boundaryStartParent && position.isBefore( this.boundaries!.start ) ) {
position = this.boundaries!.start;
} else if ( this._boundaryEndParent && position.isAfter( this.boundaries!.end ) ) {
position = this.boundaries!.end;
}

this._position = position.clone();
this._visitedParent = position.parent;
}

/**
* Gets the next tree walker's value.
*/
Expand Down
21 changes: 21 additions & 0 deletions packages/ckeditor5-engine/src/view/treewalker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,27 @@ export default class TreeWalker implements IterableIterator<TreeWalkerValue> {
}
}

/**
* Moves treewalker {@link #position} to provided `position`. Treewalker will
* continue traversing from that position.
*
* If the provided position is before the start boundary, the position will be
* set to the start boundary. If the provided position is after the end boundary,
* the position will be set to the end boundary.
* This is done to prevent the treewalker from traversing outside the boundaries.
*
* @param position Position to jump to.
*/
public jumpTo( position: Position ): void {
if ( this._boundaryStartParent && position.isBefore( this.boundaries!.start ) ) {
position = this.boundaries!.start;
} else if ( this._boundaryEndParent && position.isAfter( this.boundaries!.end ) ) {
position = this.boundaries!.end;
}

this._position = position;
}

/**
* Gets the next tree walker's value.
*
Expand Down
64 changes: 64 additions & 0 deletions packages/ckeditor5-engine/tests/model/treewalker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,70 @@ describe( 'TreeWalker', () => {
} );
} );
} );

describe( 'jumpTo', () => {
it( 'should jump to the given position', () => {
filipsobol marked this conversation as resolved.
Show resolved Hide resolved
const walker = new TreeWalker( {
startPosition: Position._createAt( paragraph, 0 )
} );

walker.jumpTo( new Position( paragraph, [ 2 ] ) );

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 3 );
} );

it( 'cannot move position before the #_boundaryStartParent', () => {
const range = new Range(
new Position( paragraph, [ 2 ] ),
new Position( paragraph, [ 4 ] )
);
const walker = new TreeWalker( {
boundaries: range
} );

const positionBeforeAllowedRange = new Position( paragraph, [ 0 ] );

walker.jumpTo( positionBeforeAllowedRange );

// `jumpTo()` autocorrected the position to the first allowed position.
expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 3 );
} );

it( 'cannot move position after the #_boundaryEndParent', () => {
const range = new Range(
new Position( paragraph, [ 0 ] ),
new Position( paragraph, [ 2 ] )
);
const walker = new TreeWalker( {
boundaries: range
} );

const positionAfterAllowedRange = new Position( paragraph, [ 4 ] );

// `jumpTo()` autocorrected the position to the last allowed position.
walker.jumpTo( positionAfterAllowedRange );

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );
} );
} );
} );

function expectValue( value, expected, options ) {
Expand Down
64 changes: 64 additions & 0 deletions packages/ckeditor5-engine/tests/view/treewalker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,70 @@ describe( 'TreeWalker', () => {
} );
} );
} );

describe( 'jumpTo', () => {
it( 'should jump to the given position', () => {
const walker = new TreeWalker( {
startPosition: Position._createAt( paragraph, 0 )
} );

walker.jumpTo( new Position( paragraph, 2 ) );

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( img2 );
expect( walker.position.offset ).to.equal( 0 );
} );

it( 'cannot move position before the #_boundaryStartParent', () => {
const range = new Range(
new Position( paragraph, 2 ),
new Position( paragraph, 4 )
);
const walker = new TreeWalker( {
boundaries: range
} );

const positionBeforeAllowedRange = new Position( paragraph, 0 );

walker.jumpTo( positionBeforeAllowedRange );

// `jumpTo()` autocorrected the position to the first allowed position.
expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( img2 );
expect( walker.position.offset ).to.equal( 0 );
} );

it( 'cannot move position after the #_boundaryEndParent', () => {
const range = new Range(
new Position( paragraph, 0 ),
new Position( paragraph, 2 )
);
const walker = new TreeWalker( {
boundaries: range
} );

const positionAfterAllowedRange = new Position( paragraph, 4 );

// `jumpTo()` autocorrected the position to the last allowed position.
walker.jumpTo( positionAfterAllowedRange );

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );

walker.next();

expect( walker.position.parent ).to.equal( paragraph );
expect( walker.position.offset ).to.equal( 2 );
} );
} );
} );

function expectValue( value, expected, options = {} ) {
Expand Down
Loading