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

@@ -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

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. */

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();

core/field.ts

@@ -1062,7 +1062,6 @@ export abstract class Field<T = any>
     this.isDirty_ = true;
     if (this.sourceBlock_ && this.sourceBlock_.rendered) {
       (this.sourceBlock_ as BlockSvg).queueRender();
-      (this.sourceBlock_ as BlockSvg).bumpNeighbours();
     }
   }
 

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;
   }
 
   /**

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;
       }

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<BlockSvg>();
@@ -13,6 +14,9 @@ const rootBlocks = new Set<BlockSvg>();
 /** The set of all blocks in need of rendering. */
 let dirtyBlocks = new WeakSet<BlockSvg>();
 
+/** A map from queued blocks to the event group from when they were queued. */
+let eventGroups = new WeakMap<BlockSvg, string>();
+
 /**
  * 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;
 }
 

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();
     }
   }
 

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 () {

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: () => {},
         },