Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add timeouts to delay expensive mutation operations #6149

Merged
merged 2 commits into from
May 12, 2022

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented May 6, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Work on #6131

Proposed Changes

Delays expensive operations using setTimeout so that:

  1. We aren't calling compose() multiple times for things that won't be rendered.
  2. We can render the mutated block without waiting for the mutator bubble to resize (which is expensive). This makes the UI feel more responsive.

Behavior Before Change

~188ms for updateWorkspace to run, which was required to complete before the first frame could be drawn.

Behavior After Change

~35ms for updateWorkspace to run, after which the mutated block is drawn
Then ~20ms for resizeBubble_ to run, after which the newly positioned/sized bubble is drawn

image
mutating-2.txt

Reason for Changes

Make the UI feel more responsive.

Test Coverage

My main concern with this was that adding the delays would cause a race condition (or similar problem) related to undo and redo. Tested using the following steps:

  1. Dragged a control_if block into the workspace.
  2. Opened the mutator, and added another input.
  3. Rapidly hit undo and redo.
  4. Observed how the undo and redo always happened in the correct order.

Tested on:

  • Desktop Chrome

Documentation

N/A

Additional Information

I tried to speed up resizeBubble_ but I couldn't figure out a way to remove the layout thrashing. The issue is our operations look like:

  1. Get the size of the mutator workspace (and its blocks) <- requires a forced layout
  2. Get the size of the block the mutator is attached to, so that the bubble doesn't obfuscate it <- requires a forced layout
  3. Resize and reposition the bubble based on 1 & 2
  4. Resize the workpace to match the new size of the bubble (updates the flyout height) <- requires a forced layout

Afaict 1 and 4 are unavoidable, unless we really want to refactor how the metrics manager works (eg having it calculate based on the width and height properties of blockSvgs, rather than using the browser's measurement).

We could get rid of 2 (which is triggered by this call) by changing this.shape_ to be a Blockly.Rect instead of an SVGElement. But this would be a breaking change.


We could also probably speed up compose and therefore updateWorkspace_ by not completely tearing down the block, which requires disposing and then re-creating all of the DOM elements associated with a block. But that's a much bigger refactor than I was looking to do.

@BeksOmega BeksOmega requested a review from a team as a code owner May 6, 2022 16:49
@BeksOmega BeksOmega changed the title fix: add timeouts to delay expensive operations fix: add timeouts to delay expensive mutation operations May 6, 2022
@BeksOmega BeksOmega merged commit 91b570a into google:develop May 12, 2022
@BeksOmega BeksOmega deleted the perf/mutate branch May 3, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants