feat: Rewrite insertion markers to improve performance and fix memory leaks#10156
Open
gonfunko wants to merge 4 commits into
Open
feat: Rewrite insertion markers to improve performance and fix memory leaks#10156gonfunko wants to merge 4 commits into
gonfunko wants to merge 4 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The basics
The details
Resolves
Fixes #5375 and #2249 once old behavior is removed.
Proposed Changes
This PR rewrites Blockly's insertion markers to be SVG groups/paths on the workspace, and not actual blocks. This fixes a long-standing leaky abstraction, and more importantly significantly improves performance when attempting to connect blocks to large stacks. It also incidentally fixes an issue where all insertion marker blocks were being leaked, causing Blockly's memory usage to grow indefinitely every time an insertion marker was shown.
Reason for Changes
Drawing insertion markers, particularly in large stacks of blocks, could be extremely slow. This is because, every time an insertion marker was shown (which was on every move), the dragging block would be cloned, the proposed connection point would be disconnected from its parent if any, and the cloned block would be connected to the proposed connection point. Connecting/disconnecting results in large DOM shuffles, because blocks are nested within each other in the DOM. This in turn caused a bunch of layout thrashing and redraw. Additionally, every insertion marker ever created was retained forever.
Now, when an insertion marker is rendered, the dragging block has its connections updated to the proposed connection (without using the connect() method, which shuffles the DOM/fires events/etc – the connection's
targetConnectionfield is just set), is rerendered to extract its path, has the path used as the insertion marker, and is rerendered in its original state. This rendering involves only the block being dragged and the block it is being connected to, and doesn't involve any DOM modification outside of the blocks having their paths set and the insertion marker being added at the root of the workspace, which don't involve shifting and invalidating the layout of large DOM trees.For users using the built-in renderers, this change will default on, and is not breaking. There are minor behavioral breaking changes possible for users with custom renderers, so this revised behavior defaults off for non-built-in renderers. Those users may opt in by setting
Blockly.InsertionMarkerPreviewer.useFastInsertionMarkers = true. This will become the default behavior in v14.This change significantly improves performance when e.g. dragging a new block into the top of the "spaghetti" block stack created in the playground in Safari, Firefox and Chrome.
Old:
https://github.com/user-attachments/assets/9c983a6c-ae7f-4e01-bcea-da9171e49242
New:
https://github.com/user-attachments/assets/f36e4fbd-1983-4ee8-951c-568f897894ce
Old profile (red flags indicate jank/dropped frames; note also continuously increasing node and listener counts):

New profile (note lack of jank, and stair-step node/listener count as GC runs):

Test Coverage
Existing tests continue to pass. I manually tested in Chrome, Safari and Firefox on Mac. I verified that insertion markers were correctly displayed when attaching blocks horizontally and vertically at the top/bottom/start/end of a stack/row, splicing them into the middle of a stack, and wrapping statement blocks around a stack, in Thrasos and Zelos, with mouse drags and keyboard-driven drags.