From 8c5f32b2f9068159ae835f3faa1b650472806a48 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 17 Jan 2024 10:48:04 -0800 Subject: [PATCH] fix: bump neighbours performance regression (#7748) * fix: move bumping neighbours to the end of rendering * chore: remove scheduleSnapAndBump * chore: remove references to bumpNeighbours * chore: work on fixing tests * fix: bump neighbours event grouping * chore: format * chore: readd deprecation import * fix: move event ordering * chore: undeprecate bumpNeighbours * fix: bumping during drag due to insertion markers * chore: tests * chore: PR feedback * chore: docs * chore: typo --- core/block.ts | 1 - core/block_dragger.ts | 2 +- core/block_svg.ts | 42 ++------------------------- core/field.ts | 1 - core/gesture.ts | 13 ++++++--- core/inputs/input.ts | 2 -- core/render_management.ts | 17 ++++++++++- core/rendered_connection.ts | 7 +++-- tests/mocha/input_test.js | 8 ----- tests/mocha/render_management_test.js | 1 + 10 files changed, 35 insertions(+), 59 deletions(-) diff --git a/core/block.ts b/core/block.ts index 53a1063d8..becc8c91b 100644 --- a/core/block.ts +++ b/core/block.ts @@ -560,7 +560,6 @@ export class Block implements IASTNodeLocation, IDeletable { * connected should not coincidentally line up on screen. */ bumpNeighbours() {} - // noop. /** * Return the parent block or null if this block is at the top level. The diff --git a/core/block_dragger.ts b/core/block_dragger.ts index ef061b6d9..0cefc4cfb 100644 --- a/core/block_dragger.ts +++ b/core/block_dragger.ts @@ -293,7 +293,7 @@ export class BlockDragger implements IBlockDragger { } else { this.draggingBlock_.queueRender(); } - this.draggingBlock_.scheduleSnapAndBump(); + this.draggingBlock_.snapToGrid(); } /** Fire a UI event at the end of a block drag. */ diff --git a/core/block_svg.ts b/core/block_svg.ts index 17e6aa3a4..5dcb1d4bd 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -142,12 +142,6 @@ export class BlockSvg private translation = ''; - /** - * The ID of the setTimeout callback for bumping neighbours, or 0 if no bump - * is currently scheduled. - */ - private bumpNeighboursPid = 0; - /** Whether this block is currently being dragged. */ private dragging = false; @@ -990,7 +984,6 @@ export class BlockSvg icon.applyColour(); icon.updateEditable(); this.queueRender(); - this.bumpNeighbours(); return icon; } @@ -1015,7 +1008,6 @@ export class BlockSvg if (type.equals(MutatorIcon.TYPE)) this.mutator = null; this.queueRender(); - this.bumpNeighbours(); return removed; } @@ -1173,7 +1165,6 @@ export class BlockSvg ) { super.setPreviousStatement(newBoolean, opt_check); this.queueRender(); - this.bumpNeighbours(); } /** @@ -1189,7 +1180,6 @@ export class BlockSvg ) { super.setNextStatement(newBoolean, opt_check); this.queueRender(); - this.bumpNeighbours(); } /** @@ -1205,7 +1195,6 @@ export class BlockSvg ) { super.setOutput(newBoolean, opt_check); this.queueRender(); - this.bumpNeighbours(); } /** @@ -1216,7 +1205,6 @@ export class BlockSvg override setInputsInline(newBoolean: boolean) { super.setInputsInline(newBoolean); this.queueRender(); - this.bumpNeighbours(); } /** @@ -1231,7 +1219,6 @@ export class BlockSvg override removeInput(name: string, opt_quiet?: boolean): boolean { const removed = super.removeInput(name, opt_quiet); this.queueRender(); - this.bumpNeighbours(); return removed; } @@ -1244,14 +1231,12 @@ export class BlockSvg override moveNumberedInputBefore(inputIndex: number, refIndex: number) { super.moveNumberedInputBefore(inputIndex, refIndex); this.queueRender(); - this.bumpNeighbours(); } /** @override */ override appendInput(input: Input): Input { super.appendInput(input); this.queueRender(); - this.bumpNeighbours(); return input; } @@ -1399,22 +1384,6 @@ export class BlockSvg * up on screen, because that creates confusion for end-users. */ override bumpNeighbours() { - if (this.bumpNeighboursPid) return; - const group = eventUtils.getGroup(); - - this.bumpNeighboursPid = setTimeout(() => { - const oldGroup = eventUtils.getGroup(); - eventUtils.setGroup(group); - this.getRootBlock().bumpNeighboursInternal(); - eventUtils.setGroup(oldGroup); - this.bumpNeighboursPid = 0; - }, config.bumpDelay); - } - - /** - * Bumps unconnected blocks out of alignment. - */ - private bumpNeighboursInternal() { const root = this.getRootBlock(); if ( this.isDeadOrDying() || @@ -1431,16 +1400,13 @@ export class BlockSvg for (const conn of this.getConnections_(false)) { if (conn.isSuperior()) { // Recurse down the block stack. - conn.targetBlock()?.bumpNeighboursInternal(); + conn.targetBlock()?.bumpNeighbours(); } for (const neighbour of conn.neighbours(config.snapRadius)) { - // Don't bump away from things that are in our stack. if (neighbourIsInStack(neighbour)) continue; - // If both connections are connected, that's fine. if (conn.isConnected() && neighbour.isConnected()) continue; - // Always bump the inferior connection. if (conn.isSuperior()) { neighbour.bumpAwayFrom(conn); } else { @@ -1451,10 +1417,8 @@ export class BlockSvg } /** - * Schedule snapping to grid and bumping neighbours to occur after a brief - * delay. - * - * @internal + * Snap to grid, and then bump neighbouring blocks away at the end of the next + * render. */ scheduleSnapAndBump() { this.snapToGrid(); diff --git a/core/field.ts b/core/field.ts index 7522cde93..4ee4b1763 100644 --- a/core/field.ts +++ b/core/field.ts @@ -1062,7 +1062,6 @@ export abstract class Field this.isDirty_ = true; if (this.sourceBlock_ && this.sourceBlock_.rendered) { (this.sourceBlock_ as BlockSvg).queueRender(); - (this.sourceBlock_ as BlockSvg).bumpNeighbours(); } } diff --git a/core/gesture.ts b/core/gesture.ts index 4b85c4f6f..2aed48d79 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -125,6 +125,9 @@ export class Gesture { */ private workspaceDragger: WorkspaceDragger | null = null; + /** Whether the gesture is dragging or not. */ + private dragging: boolean = false; + /** The flyout a gesture started in, if any. */ private flyout: IFlyout | null = null; @@ -369,6 +372,7 @@ export class Gesture { : this.startWorkspace_ && this.startWorkspace_.isDraggable(); if (!wsMovable) return; + this.dragging = true; this.workspaceDragger = new WorkspaceDragger(this.startWorkspace_); this.workspaceDragger.startDrag(); @@ -408,6 +412,7 @@ export class Gesture { true, ); + this.dragging = true; this.blockDragger = new BlockDraggerClass!( this.targetBlock, this.startWorkspace_, @@ -431,6 +436,7 @@ export class Gesture { ); } + this.dragging = true; this.bubbleDragger = new BubbleDragger( this.startBubble, this.startWorkspace_, @@ -963,7 +969,8 @@ export class Gesture { eventUtils.setGroup(true); } const newBlock = this.flyout.createBlock(this.targetBlock); - newBlock.scheduleSnapAndBump(); + newBlock.snapToGrid(); + newBlock.bumpNeighbours(); } } else { if (!this.startWorkspace_) { @@ -1205,9 +1212,7 @@ export class Gesture { * @internal */ isDragging(): boolean { - return ( - !!this.workspaceDragger || !!this.blockDragger || !!this.bubbleDragger - ); + return this.dragging; } /** diff --git a/core/inputs/input.ts b/core/inputs/input.ts index a94102aca..bcf8cab78 100644 --- a/core/inputs/input.ts +++ b/core/inputs/input.ts @@ -124,7 +124,6 @@ export class Input { if (this.sourceBlock.rendered) { (this.sourceBlock as BlockSvg).queueRender(); - this.sourceBlock.bumpNeighbours(); } return index; } @@ -145,7 +144,6 @@ export class Input { this.fieldRow.splice(i, 1); if (this.sourceBlock.rendered) { (this.sourceBlock as BlockSvg).queueRender(); - this.sourceBlock.bumpNeighbours(); } return true; } diff --git a/core/render_management.ts b/core/render_management.ts index 541459860..5aadc563c 100644 --- a/core/render_management.ts +++ b/core/render_management.ts @@ -6,6 +6,7 @@ import {BlockSvg} from './block_svg.js'; import * as userAgent from './utils/useragent.js'; +import * as eventUtils from './events/utils.js'; /** The set of all blocks in need of rendering which don't have parents. */ const rootBlocks = new Set(); @@ -13,6 +14,9 @@ const rootBlocks = new Set(); /** The set of all blocks in need of rendering. */ let dirtyBlocks = new WeakSet(); +/** A map from queued blocks to the event group from when they were queued. */ +let eventGroups = new WeakMap(); + /** * The promise which resolves after the current set of renders is completed. Or * null if there are no queued renders. @@ -100,6 +104,7 @@ function alwaysImmediatelyRender() { */ function queueBlock(block: BlockSvg) { dirtyBlocks.add(block); + eventGroups.set(block, eventUtils.getGroup()); const parent = block.getParent(); if (parent) { queueBlock(parent); @@ -124,9 +129,19 @@ function doRenders() { const blockOrigin = block.getRelativeToSurfaceXY(); block.updateComponentLocations(blockOrigin); } + for (const block of blocks) { + const oldGroup = eventUtils.getGroup(); + const newGroup = eventGroups.get(block); + if (newGroup) eventUtils.setGroup(newGroup); + + block.bumpNeighbours(); + + eventUtils.setGroup(oldGroup); + } rootBlocks.clear(); - dirtyBlocks = new Set(); + dirtyBlocks = new WeakSet(); + eventGroups = new WeakMap(); afterRendersPromise = null; } diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index e68450883..61cc56731 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -499,6 +499,9 @@ export class RenderedConnection extends Connection { const {parentConnection, childConnection} = this.getParentAndChildConnections(); if (!parentConnection || !childConnection) return; + const existingGroup = eventUtils.getGroup(); + if (!existingGroup) eventUtils.setGroup(true); + const parent = parentConnection.getSourceBlock() as BlockSvg; const child = childConnection.getSourceBlock() as BlockSvg; super.disconnectInternal(setParent); @@ -508,6 +511,8 @@ export class RenderedConnection extends Connection { child.queueRender(); // Reset visibility, since the child is now a top block. child.getSvgRoot().style.display = 'block'; + + eventUtils.setGroup(existingGroup); } /** @@ -579,8 +584,6 @@ export class RenderedConnection extends Connection { ) { const child = this.isSuperior() ? this.targetBlock() : this.sourceBlock_; child!.unplug(); - // Bump away. - this.sourceBlock_.bumpNeighbours(); } } diff --git a/tests/mocha/input_test.js b/tests/mocha/input_test.js index c4ecd7a39..73baf20f6 100644 --- a/tests/mocha/input_test.js +++ b/tests/mocha/input_test.js @@ -27,14 +27,12 @@ suite('Inputs', function () { ); this.renderStub = sinon.stub(this.block, 'queueRender'); - this.bumpNeighboursStub = sinon.stub(this.block, 'bumpNeighbours'); this.dummy = this.block.appendDummyInput('DUMMY'); this.value = this.block.appendValueInput('VALUE'); this.statement = this.block.appendStatementInput('STATEMENT'); this.renderStub.resetHistory(); - this.bumpNeighboursStub.resetHistory(); }); teardown(function () { sharedTestTeardown.call(this); @@ -158,7 +156,6 @@ suite('Inputs', function () { chai.assert.equal(setBlockSpy.getCall(0).args[0], this.block); sinon.assert.calledOnce(initSpy); sinon.assert.calledOnce(this.renderStub); - sinon.assert.calledOnce(this.bumpNeighboursStub); setBlockSpy.restore(); initSpy.restore(); @@ -177,7 +174,6 @@ suite('Inputs', function () { chai.assert.equal(setBlockSpy.getCall(0).args[0], this.block); sinon.assert.calledOnce(initModelSpy); sinon.assert.notCalled(this.renderStub); - sinon.assert.notCalled(this.bumpNeighboursStub); setBlockSpy.restore(); initModelSpy.restore(); @@ -196,12 +192,10 @@ suite('Inputs', function () { this.dummy.appendField(field, 'FIELD'); this.renderStub.resetHistory(); - this.bumpNeighboursStub.resetHistory(); this.dummy.removeField('FIELD'); sinon.assert.calledOnce(disposeSpy); sinon.assert.calledOnce(this.renderStub); - sinon.assert.calledOnce(this.bumpNeighboursStub); }); test('Headless', function () { const field = new Blockly.FieldLabel('field'); @@ -209,14 +203,12 @@ suite('Inputs', function () { this.dummy.appendField(field, 'FIELD'); this.renderStub.resetHistory(); - this.bumpNeighboursStub.resetHistory(); this.block.rendered = false; this.dummy.removeField('FIELD'); sinon.assert.calledOnce(disposeSpy); sinon.assert.notCalled(this.renderStub); - sinon.assert.notCalled(this.bumpNeighboursStub); }); }); suite('Field Ordering/Manipulation', function () { diff --git a/tests/mocha/render_management_test.js b/tests/mocha/render_management_test.js index d5d957df4..971ed1667 100644 --- a/tests/mocha/render_management_test.js +++ b/tests/mocha/render_management_test.js @@ -32,6 +32,7 @@ suite('Render Management', function () { isDisposed: () => false, getRelativeToSurfaceXY: () => ({x: 0, y: 0}), updateComponentLocations: () => {}, + bumpNeighbours: () => {}, workspace: { resizeContents: () => {}, },