From e80e213e7f6ecf0c3cbc65f085abc69430024ab2 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Wed, 27 Nov 2024 15:46:23 +0100 Subject: [PATCH 1/3] Improve performance of `_insertNodes` --- .../ckeditor5-engine/src/model/nodelist.ts | 15 +++--- packages/ckeditor5-utils/src/splicearray.ts | 48 ++++++++----------- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/nodelist.ts b/packages/ckeditor5-engine/src/model/nodelist.ts index 945d1af9768..2f542d8fa64 100644 --- a/packages/ckeditor5-engine/src/model/nodelist.ts +++ b/packages/ckeditor5-engine/src/model/nodelist.ts @@ -164,6 +164,8 @@ export default class NodeList implements Iterable { * @param nodes Nodes to be inserted. */ public _insertNodes( index: number, nodes: Iterable ): void { + const nodesArray: Array = []; + // Validation. for ( const node of nodes ) { if ( !( node instanceof Node ) ) { @@ -174,16 +176,15 @@ export default class NodeList implements Iterable { */ throw new CKEditorError( 'model-nodelist-insertnodes-not-node', this ); } - } - const nodesArray = Array.from( nodes ); - const offsetsArray = makeOffsetsArray( nodesArray ); + nodesArray.push( node ); + } let offset = this.indexToOffset( index ); // Splice nodes array and offsets array into the nodelist. - this._nodes = spliceArray( this._nodes, nodesArray, index, 0 ); - this._offsetToNode = spliceArray( this._offsetToNode, offsetsArray, offset, 0 ); + spliceArray( this._nodes, nodesArray, index ); + spliceArray( this._offsetToNode, makeOffsetsArray( nodesArray ), offset ); // Refresh indexes and offsets for nodes inside this node list. We need to do this for all inserted nodes and all nodes after them. for ( let i = index; i < this._nodes.length; i++ ) { @@ -231,7 +232,9 @@ export default class NodeList implements Iterable { } /** - * Removes children nodes provided as an array. + * Removes children nodes provided as an array. These nodes do not need to be direct siblings. + * + * This method is faster than removing nodes one by one, as it recalculates offsets only once. * * @internal * @param nodes Array of nodes. diff --git a/packages/ckeditor5-utils/src/splicearray.ts b/packages/ckeditor5-utils/src/splicearray.ts index 8b813e9da67..1f8c44679f2 100644 --- a/packages/ckeditor5-utils/src/splicearray.ts +++ b/packages/ckeditor5-utils/src/splicearray.ts @@ -8,17 +8,15 @@ */ /** - * Splices one array into another. To be used instead of `Array.prototype.splice` as the latter may - * throw "Maximum call stack size exceeded" when passed huge number of items to insert. - * - * Note: in contrary to Array.splice, this function does not modify the original `target`. + * Splices one array into another. To be used instead of `Array.prototype.splice` for better + * performance and because the latter may throw "Maximum call stack size exceeded" error when + * passing huge number of items to insert. * * ```ts - * spliceArray( [ 1, 2 ], [ 3, 4 ], 0, 0 ); // [ 3, 4, 1, 2 ] - * spliceArray( [ 1, 2 ], [ 3, 4 ], 1, 1 ); // [ 1, 3, 4 ] - * spliceArray( [ 1, 2 ], [ 3, 4 ], 1, 0 ); // [ 1, 3, 4, 2 ] - * spliceArray( [ 1, 2 ], [ 3, 4 ], 2, 0 ); // [ 1, 2, 3, 4 ] - * spliceArray( [ 1, 2 ], [], 0, 1 ); // [ 2 ] + * spliceArray( [ 1, 2 ], [ 3, 4 ], 0 ); // [ 3, 4, 1, 2 ] + * spliceArray( [ 1, 2 ], [ 3, 4 ], 1 ); // [ 1, 3, 4, 2 ] + * spliceArray( [ 1, 2 ], [ 3, 4 ], 2 ); // [ 1, 2, 3, 4 ] + * spliceArray( [ 1, 2 ], [], 0 ); // [ 1, 2 ] * ``` * * @param target Array to be spliced. @@ -29,28 +27,20 @@ * @returns New spliced array. */ export default function spliceArray( - target: ReadonlyArray, - source: ReadonlyArray, - start: number, - count: number -): Array { - const targetLength = target.length; - const sourceLength = source.length; - const result = new Array( targetLength - count + sourceLength ); - - let i = 0; + targetArray: Array, + insertArray: Array, + index: number +): void { + const originalLength = targetArray.length; + const insertLength = insertArray.length; - for ( ; i < start && i < targetLength; i++ ) { - result[ i ] = target[ i ]; + // Shift elements in the target array to make space for insertArray + for ( let i = originalLength - 1; i >= index; i-- ) { + targetArray[ i + insertLength ] = targetArray[ i ]; } - for ( let j = 0; j < sourceLength; j++, i++ ) { - result[ i ] = source[ j ]; + // Copy elements from insertArray into the target array + for ( let i = 0; i < insertLength; i++ ) { + targetArray[ index + i ] = insertArray[ i ]; } - - for ( let k = start + count; k < targetLength; k++, i++ ) { - result[ i ] = target[ k ]; - } - - return result; } From 60068e0e48940f77f79eac0897b3bc7009cded8d Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Wed, 27 Nov 2024 15:48:04 +0100 Subject: [PATCH 2/3] Update comment --- packages/ckeditor5-engine/src/model/nodelist.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/nodelist.ts b/packages/ckeditor5-engine/src/model/nodelist.ts index 2f542d8fa64..1a5b0424190 100644 --- a/packages/ckeditor5-engine/src/model/nodelist.ts +++ b/packages/ckeditor5-engine/src/model/nodelist.ts @@ -232,9 +232,7 @@ export default class NodeList implements Iterable { } /** - * Removes children nodes provided as an array. These nodes do not need to be direct siblings. - * - * This method is faster than removing nodes one by one, as it recalculates offsets only once. + * Removes children nodes provided as an array. * * @internal * @param nodes Array of nodes. From 397fccfb510d54f2ad3b7ecb19d6b3af17520bf8 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Wed, 27 Nov 2024 15:59:31 +0100 Subject: [PATCH 3/3] Update tests --- packages/ckeditor5-utils/tests/splicearray.js | 31 +++++++------------ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/packages/ckeditor5-utils/tests/splicearray.js b/packages/ckeditor5-utils/tests/splicearray.js index cbb7a850ab4..4e50f6f5c26 100644 --- a/packages/ckeditor5-utils/tests/splicearray.js +++ b/packages/ckeditor5-utils/tests/splicearray.js @@ -11,36 +11,27 @@ describe( 'utils', () => { const target = [ 1, 2 ]; const source = [ 3, 4 ]; - const result = spliceArray( target, source, 0, 0 ); + spliceArray( target, source, 0 ); - expect( result ).to.have.members( [ 3, 4, 1, 2 ] ); + expect( target ).to.have.members( [ 3, 4, 1, 2 ] ); } ); it( 'should insert elements in the middle of the target array', () => { const target = [ 1, 2 ]; const source = [ 3, 4 ]; - const result = spliceArray( target, source, 1, 0 ); + spliceArray( target, source, 1 ); - expect( result ).to.have.members( [ 1, 3, 4, 2 ] ); + expect( target ).to.have.members( [ 1, 3, 4, 2 ] ); } ); it( 'should insert elements at the end of the target array', () => { const target = [ 1, 2 ]; const source = [ 3, 4 ]; - const result = spliceArray( target, source, 2, 0 ); + spliceArray( target, source, 2 ); - expect( result ).to.have.members( [ 1, 2, 3, 4 ] ); - } ); - - it( 'should only splice target array when source is empty', () => { - const target = [ 1, 2 ]; - const source = []; - - const result = spliceArray( target, source, 0, 1 ); - - expect( result ).to.have.members( [ 2 ] ); + expect( target ).to.have.members( [ 1, 2, 3, 4 ] ); } ); it( 'should insert elements into array which contains a large number of elements (250 000)', () => { @@ -48,19 +39,19 @@ describe( 'utils', () => { const source = [ 'b', 'c' ]; const expectedLength = target.length + source.length; - const result = spliceArray( target, source, 0, 0 ); + spliceArray( target, source, 0 ); - expect( result.length ).to.equal( expectedLength ); - expect( result[ 0 ] ).to.equal( source[ 0 ] ); + expect( target.length ).to.equal( expectedLength ); + expect( target[ 0 ] ).to.equal( source[ 0 ] ); } ); it( 'should insert elements in the middle of the target array which contains a large number of elements (250 000)', () => { const target = 'a'.repeat( 250000 ).split( '' ); const source = [ 'b', 'c' ]; - const result = spliceArray( target, source, 5, 0 ); + spliceArray( target, source, 5 ); - expect( result[ 5 ] ).to.equal( source[ 0 ] ); + expect( target[ 5 ] ).to.equal( source[ 0 ] ); } ); } ); } );