From 0badbb9a260c1268ba4bb9e32080f9197ddd31b6 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 30 Jul 2024 13:58:25 +0200 Subject: [PATCH 1/6] Fix differ crashing on large paragraphs --- packages/ckeditor5-engine/src/model/differ.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/model/differ.ts b/packages/ckeditor5-engine/src/model/differ.ts index 41e31682b69..1802d8b3724 100644 --- a/packages/ckeditor5-engine/src/model/differ.ts +++ b/packages/ckeditor5-engine/src/model/differ.ts @@ -1520,7 +1520,9 @@ function _generateDiffInstructionsFromChanges( oldChildrenLength: number, change // We removed `howMany` old nodes, update `oldChildrenHandled`. oldChildrenHandled += change.howMany; } else { - diff.push( ...'a'.repeat( change.howMany ).split( '' ) ); + for ( let i = 0; i < change.howMany; i++ ) { + diff.push( 'a' ); + } // The last handled offset is at the position after the changed range. offset = change.offset + change.howMany; From 78f6c8f2c86ce0b9c6d23798904603f0877912d9 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 2 Aug 2024 12:06:21 +0200 Subject: [PATCH 2/6] Decide which way we are pushing appended items depending on length of change --- packages/ckeditor5-engine/src/model/differ.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/differ.ts b/packages/ckeditor5-engine/src/model/differ.ts index 1802d8b3724..68bcde913e6 100644 --- a/packages/ckeditor5-engine/src/model/differ.ts +++ b/packages/ckeditor5-engine/src/model/differ.ts @@ -1520,8 +1520,15 @@ function _generateDiffInstructionsFromChanges( oldChildrenLength: number, change // We removed `howMany` old nodes, update `oldChildrenHandled`. oldChildrenHandled += change.howMany; } else { - for ( let i = 0; i < change.howMany; i++ ) { - diff.push( 'a' ); + // Total maximum amount of arguments that can be passed to `Array.prototype.push` may be limited so we need to + // add them manually one by one to avoid this limit. However loop might be a bit slower than `push` method on + // smaller changesets so we need to decide which method to use based on the size of the change. + if ( change.howMany > 500 ) { + for ( let i = 0; i < change.howMany; i++ ) { + diff.push( 'a' ); + } + } else { + diff.push( ...'a'.repeat( change.howMany ).split( '' ) ); } // The last handled offset is at the position after the changed range. From c4843e47d17bb773141a1f6168a1e78d31c41e92 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 2 Aug 2024 13:34:19 +0200 Subject: [PATCH 3/6] Add tests --- .../ckeditor5-engine/tests/model/differ.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/packages/ckeditor5-engine/tests/model/differ.js b/packages/ckeditor5-engine/tests/model/differ.js index e47e3bc14a3..75fc4c889cb 100644 --- a/packages/ckeditor5-engine/tests/model/differ.js +++ b/packages/ckeditor5-engine/tests/model/differ.js @@ -1707,6 +1707,62 @@ describe( 'Differ', () => { expectChanges( [] ); } ); } ); + + // See: https://github.com/ckeditor/ckeditor5/issues/16819 + it( 'on element with very long text should have batched instructions together during generating diff from changes', () => { + const MAX_PUSH_CALL_STACK_ARGS = 500; + + const p = root.getChild( 0 ); + const veryLongString = 'a'.repeat( 300 ); + + model.change( () => { + insert( veryLongString, Position._createAt( p, 0 ) ); + } ); + + const pushSpy = sinon.spy( Array.prototype, 'push' ); + + model.change( writer => { + writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) ); + } ); + + // Let's check if append instructions has been batched together in single `.push()` call. + const instructionsDiff = pushSpy.args.find( args => ( + args.length >= 300 && + args.length < MAX_PUSH_CALL_STACK_ARGS && + args.every( ch => ch === 'a' ) + ) ); + + expect( instructionsDiff ).not.to.be.undefined; + pushSpy.restore(); + } ); + + // See: https://github.com/ckeditor/ckeditor5/issues/16819 + it( 'on element with very long text should not have batched instructions together during generating diff from changes ' + + 'that are larger than max push call stack args count', () => { + const MAX_PUSH_CALL_STACK_ARGS = 500; + + const p = root.getChild( 0 ); + const veryLongString = 'a'.repeat( MAX_PUSH_CALL_STACK_ARGS + 10 ); + + model.change( () => { + insert( veryLongString, Position._createAt( p, 0 ) ); + } ); + + const pushSpy = sinon.spy( Array.prototype, 'push' ); + + model.change( writer => { + writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) ); + } ); + + // Let's check if append instructions has been NOT batched together in single `.push()` call. + const instructionsDiff = pushSpy.args.find( args => ( + args.length > MAX_PUSH_CALL_STACK_ARGS && + args.every( ch => ch === 'a' ) + ) ); + + expect( instructionsDiff ).to.be.undefined; + pushSpy.restore(); + } ); } ); describe( 'split', () => { From 080c34f7b411710b7957c9ccfb0c5023de190a86 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 2 Aug 2024 13:37:03 +0200 Subject: [PATCH 4/6] Adjust doc --- packages/ckeditor5-engine/src/model/differ.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ckeditor5-engine/src/model/differ.ts b/packages/ckeditor5-engine/src/model/differ.ts index 68bcde913e6..281ade6700b 100644 --- a/packages/ckeditor5-engine/src/model/differ.ts +++ b/packages/ckeditor5-engine/src/model/differ.ts @@ -1523,6 +1523,7 @@ function _generateDiffInstructionsFromChanges( oldChildrenLength: number, change // Total maximum amount of arguments that can be passed to `Array.prototype.push` may be limited so we need to // add them manually one by one to avoid this limit. However loop might be a bit slower than `push` method on // smaller changesets so we need to decide which method to use based on the size of the change. + // See: https://github.com/ckeditor/ckeditor5/issues/16819 if ( change.howMany > 500 ) { for ( let i = 0; i < change.howMany; i++ ) { diff.push( 'a' ); From 9c298a37f486ee2e0ef282b6b688a085afe3aef0 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 2 Aug 2024 13:38:01 +0200 Subject: [PATCH 5/6] Change limit --- packages/ckeditor5-engine/src/model/differ.ts | 2 +- packages/ckeditor5-engine/tests/model/differ.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-engine/src/model/differ.ts b/packages/ckeditor5-engine/src/model/differ.ts index 281ade6700b..bb6b8bb0d3e 100644 --- a/packages/ckeditor5-engine/src/model/differ.ts +++ b/packages/ckeditor5-engine/src/model/differ.ts @@ -1524,7 +1524,7 @@ function _generateDiffInstructionsFromChanges( oldChildrenLength: number, change // add them manually one by one to avoid this limit. However loop might be a bit slower than `push` method on // smaller changesets so we need to decide which method to use based on the size of the change. // See: https://github.com/ckeditor/ckeditor5/issues/16819 - if ( change.howMany > 500 ) { + if ( change.howMany > 1500 ) { for ( let i = 0; i < change.howMany; i++ ) { diff.push( 'a' ); } diff --git a/packages/ckeditor5-engine/tests/model/differ.js b/packages/ckeditor5-engine/tests/model/differ.js index 75fc4c889cb..47c711b6e04 100644 --- a/packages/ckeditor5-engine/tests/model/differ.js +++ b/packages/ckeditor5-engine/tests/model/differ.js @@ -1710,7 +1710,7 @@ describe( 'Differ', () => { // See: https://github.com/ckeditor/ckeditor5/issues/16819 it( 'on element with very long text should have batched instructions together during generating diff from changes', () => { - const MAX_PUSH_CALL_STACK_ARGS = 500; + const MAX_PUSH_CALL_STACK_ARGS = 1500; const p = root.getChild( 0 ); const veryLongString = 'a'.repeat( 300 ); @@ -1739,7 +1739,7 @@ describe( 'Differ', () => { // See: https://github.com/ckeditor/ckeditor5/issues/16819 it( 'on element with very long text should not have batched instructions together during generating diff from changes ' + 'that are larger than max push call stack args count', () => { - const MAX_PUSH_CALL_STACK_ARGS = 500; + const MAX_PUSH_CALL_STACK_ARGS = 1500; const p = root.getChild( 0 ); const veryLongString = 'a'.repeat( MAX_PUSH_CALL_STACK_ARGS + 10 ); From 67f59434f0294d61ebf183953040409f98bf0c0a Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 2 Aug 2024 14:11:46 +0200 Subject: [PATCH 6/6] Fix typo --- packages/ckeditor5-engine/tests/model/differ.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/tests/model/differ.js b/packages/ckeditor5-engine/tests/model/differ.js index 47c711b6e04..cc7b388288c 100644 --- a/packages/ckeditor5-engine/tests/model/differ.js +++ b/packages/ckeditor5-engine/tests/model/differ.js @@ -1725,7 +1725,7 @@ describe( 'Differ', () => { writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) ); } ); - // Let's check if append instructions has been batched together in single `.push()` call. + // Let's check if appended instructions has been batched together in single `.push()` call. const instructionsDiff = pushSpy.args.find( args => ( args.length >= 300 && args.length < MAX_PUSH_CALL_STACK_ARGS && @@ -1754,7 +1754,7 @@ describe( 'Differ', () => { writer.setAttribute( attributeKey, true, writer.createRangeIn( p ) ); } ); - // Let's check if append instructions has been NOT batched together in single `.push()` call. + // Let's check if appended instructions has been NOT batched together in single `.push()` call. const instructionsDiff = pushSpy.args.find( args => ( args.length > MAX_PUSH_CALL_STACK_ARGS && args.every( ch => ch === 'a' )