fix: dispose performance (#6894)

* fix: improve dispose performance

* chore: cleanup dispose functions

* chore: split dispose into dispose and disposeInternal

* chore: remove unnecessary node removal

* fix: remove unnecessary unbinding of event listeners

* fix: readd skipping event construction

* chore: work on fixing tests

* chore: fix remaining test failures

* chore: format

* chore: typo

* fix: first pass of PR comments

* chore: remove TODO

core/block.ts

@@ -312,44 +312,45 @@ export class Block implements IASTNodeLocation, IDeletable {
    * @suppress {checkTypes}
    */
   dispose(healStack: boolean) {
-    if (this.isDeadOrDying()) {
-      return;
-    }
-    // Terminate onchange event calls.
+    if (this.isDeadOrDying()) return;
+
+    // Dispose of this change listener before unplugging.
+    // Technically not necessary due to the event firing delay.
+    // But future-proofing.
     if (this.onchangeWrapper_) {
       this.workspace.removeChangeListener(this.onchangeWrapper_);
     }
 
     this.unplug(healStack);
     if (eventUtils.isEnabled()) {
+      // Constructing the delete event is costly. Only perform if necessary.
       eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_DELETE))(this));
     }
-    eventUtils.disable();
+    this.workspace.removeTopBlock(this);
+    this.disposeInternal();
+  }
 
+  /**
+   * Disposes of this block without doing things required by the top block.
+   * E.g. does not fire events, unplug the block, etc.
+   */
+  protected disposeInternal() {
+    if (this.isDeadOrDying()) return;
+
+    if (this.onchangeWrapper_) {
+      this.workspace.removeChangeListener(this.onchangeWrapper_);
+    }
+
+    eventUtils.disable();
     try {
-      // This block is now at the top of the workspace.
-      // Remove this block from the workspace's list of top-most blocks.
-      this.workspace.removeTopBlock(this);
       this.workspace.removeTypedBlock(this);
-      // Remove from block database.
       this.workspace.removeBlockById(this.id);
       this.disposing = true;
 
-      // First, dispose of all my children.
-      for (let i = this.childBlocks_.length - 1; i >= 0; i--) {
-        this.childBlocks_[i].dispose(false);
-      }
-      // Then dispose of myself.
-      // Dispose of all inputs and their fields.
-      for (let i = 0, input; input = this.inputList[i]; i++) {
-        input.dispose();
-      }
+      this.childBlocks_.forEach((c) => c.disposeInternal());
+      this.inputList.forEach((i) => i.dispose());
       this.inputList.length = 0;
-      // Dispose of any remaining connections (next/previous/output).
-      const connections = this.getConnections_(true);
-      for (let i = 0, connection; connection = connections[i]; i++) {
-        connection.dispose();
-      }
+      this.getConnections_(true).forEach((c) => c.dispose());
     } finally {
       eventUtils.enable();
       if (typeof this.destroy === 'function') {

core/block_animations.ts

@@ -39,6 +39,9 @@ let wobblingBlock: BlockSvg|null = null;
  * @internal
  */
 export function disposeUiEffect(block: BlockSvg) {
+  // Disposing is going to take so long the animation won't play anyway.
+  if (block.getDescendants(false).length > 100) return;
+
   const workspace = block.workspace;
   const svgGroup = block.getSvgRoot();
   workspace.getAudioManager().play('delete');

core/block_svg.ts

@@ -841,55 +841,39 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
    * @suppress {checkTypes}
    */
   override dispose(healStack?: boolean, animate?: boolean) {
-    if (this.isDeadOrDying()) {
-      return;
-    }
+    if (this.isDeadOrDying()) return;
+
     Tooltip.dispose();
-    Tooltip.unbindMouseEvents(this.pathObject.svgPath);
-    dom.startTextWidthCache();
-    // Save the block's workspace temporarily so we can resize the
-    // contents once the block is disposed.
-    const blockWorkspace = this.workspace;
-    // If this block is being dragged, unlink the mouse events.
-    if (common.getSelected() === this) {
-      this.unselect();
-      this.workspace.cancelCurrentGesture();
-    }
-    // If this block has a context menu open, close it.
-    if (ContextMenu.getCurrentBlock() === this) {
-      ContextMenu.hide();
-    }
+    ContextMenu.hide();
 
     if (animate && this.rendered) {
       this.unplug(healStack);
       blockAnimations.disposeUiEffect(this);
     }
-    // Stop rerendering.
-    this.rendered = false;
-
-    // Clear pending warnings.
-    for (const n of this.warningTextDb.values()) {
-      clearTimeout(n);
-    }
-    this.warningTextDb.clear();
-
-    const icons = this.getIcons();
-    for (let i = 0; i < icons.length; i++) {
-      icons[i].dispose();
-    }
-
-    // Just deleting this block from the DOM would result in a memory leak as
-    // well as corruption of the connection database.  Therefore we must
-    // methodically step through the blocks and carefully disassemble them.
-    if (common.getSelected() === this) {
-      common.setSelected(null);
-    }
 
     super.dispose(!!healStack);
-
     dom.removeNode(this.svgGroup_);
-    blockWorkspace.resizeContents();
-    dom.stopTextWidthCache();
+  }
+
+  /**
+   * Disposes of this block without doing things required by the top block.
+   * E.g. does trigger UI effects, remove nodes, etc.
+   */
+  override disposeInternal() {
+    if (this.isDeadOrDying()) return;
+    super.disposeInternal();
+
+    this.rendered = false;
+
+    if (common.getSelected() === this) {
+      this.unselect();
+      this.workspace.cancelCurrentGesture();
+    }
+
+    [...this.warningTextDb.values()].forEach((n) => clearTimeout(n));
+    this.warningTextDb.clear();
+
+    this.getIcons().forEach((i) => i.dispose());
   }
 
   /**

core/connection.ts

@@ -150,7 +150,7 @@ export class Connection implements IASTNodeLocationWithBlock {
       this.setShadowStateInternal_();
 
       const targetBlock = this.targetBlock();
-      if (targetBlock) {
+      if (targetBlock && !targetBlock.isDeadOrDying()) {
         // Disconnect the attached normal block.
         targetBlock.unplug();
       }
@@ -552,6 +552,7 @@ export class Connection implements IASTNodeLocationWithBlock {
       }
     } else if (target.isShadow()) {
       target.dispose(false);
+      if (this.getSourceBlock().isDeadOrDying()) return;
       this.respawnShadow_();
       if (this.targetBlock() && this.targetBlock()!.isShadow()) {
         this.serializeShadow_(this.targetBlock());

core/field.ts

@@ -489,14 +489,11 @@ export abstract class Field<T = any> implements IASTNodeLocationSvg,
   dispose() {
     dropDownDiv.hideIfOwner(this);
     WidgetDiv.hideIfOwner(this);
-    Tooltip.unbindMouseEvents(this.getClickTarget_());
 
-    if (this.mouseDownWrapper_) {
-      browserEvents.unbind(this.mouseDownWrapper_);
+    if (!this.getSourceBlock()?.isDeadOrDying()) {
+      dom.removeNode(this.fieldGroup_);
     }
 
-    dom.removeNode(this.fieldGroup_);
-
     this.disposed = true;
   }
 

core/icon.ts

@@ -79,8 +79,10 @@ export abstract class Icon {
 
   /** Dispose of this icon. */
   dispose() {
-    dom.removeNode(this.iconGroup_);  // Dispose of and unlink the icon.
-    this.setVisible(false);           // Dispose of and unlink the bubble.
+    if (!this.getBlock().isDeadOrDying()) {
+      dom.removeNode(this.iconGroup_);
+    }
+    this.setVisible(false);  // Dispose of and unlink the bubble.
   }
 
   /** Add or remove the UI indicating if this icon may be clicked or not. */

core/tooltip.ts

@@ -276,6 +276,7 @@ function onMouseOut(_e: PointerEvent) {
     hide();
   }, 1);
   clearTimeout(showPid);
+  showPid = 0;
 }
 
 /**
@@ -342,6 +343,7 @@ export function hide() {
   }
   if (showPid) {
     clearTimeout(showPid);
+    showPid = 0;
   }
 }