Scratch-blocks: Performance: opening the toolbox

[rachel-fenichel]

Switching toolbox categories takes far longer than it ought to, given that we have a mostly static set of blocks. This isn't really surprising since we've never tried to optimize that code.

Some ideas and potential low hanging fruit:

• Better caching of computed text length for strings
• Avoid spurious rerendering of fields
• For fully static categories, cache block coordinates instead of needing a layout pass
  
  
  
  • This might not actually be an improvement; I need to look into it.
  
  

[thisandagain]

@rachel-fenichel and @picklesrus is this something you folks could help us with?

[rachel-fenichel]

Yes, it's something that I'm thinking about but don't have any patches yet.

[picklesrus]

Yeah, as @rachel-fenichel mentioned, we haven't tried to optimize it and there is likely lots of low hanging fruit. It might be a while til I could look at this in depth, but I'll try and keep it top of mind. Here's one thing I noticed while in the code today:

Flyout's getMetrics calls getBBox() on the block canvas which, iirc, causes a layout pass. We should be able to replace that with something like getBlocksBoundingBox which uses cached blocks sizes. I haven't looked into how often we call Flyout's getMetrics though so it might just be a minor win.

[samelhusseini]

We took a brief look into the "spurious rerendering of fields". It looks like field::init, the size calculations are causing the field render_ function to be called three times (once in getSize, once in render(), and once because the width is set to zero).

Although, this all looks intentional..

[picklesrus]

Minor update

Not calling getComputedTextLength all.the.time. is still the biggest win we can get. Plus it is applicable across almost every block/field.

Despite that, I dove down some other rabbit holes anyway so here are some notes:

I tested out the previously mentioned BBox change with #1059 patched in and it does remove a re-calculate style on every animation frame when choosing a new category. It should also remove one on every mouse move while scrolling within the flyout as well as anytime we call flyout.show. I have a few details (vertical vs horizontal, rtl, etc.) to sort out, but they seem straightforward enough that I should just push ahead and do it.

These things also struck me as odd:

• Why does disposing of a flyout category worth of blocks take 1/2 the frame budget (8ms) despite there being no apparent redraws/reflows happening? (disclaimer: these numbers come from my aging, fan running, macbook air)
• in field_dropdown, why do we remove things from the dom during render? This causes a reflow. Can we just display:none/visibility:hidden the element instead and reset its text/content when the dropdown is closed?

[picklesrus]

FYI: the flyout bbox thing isn't quite as easy as the afternoon PR I hoped it would be, but it is still pretty straightforward. Filed https://github.com/google/blockly/issues/1303 upstream to track progress.

[paulkaplan]

I did some investigation of this this morning, focusing mainly on the layout thrashing (i.e. reading from and writing to the DOM in a way that forces a layout), which seems to be a pretty large chunk of the time spent in showing blocks.

Leaving the field cache open reduced a considerable amount of thrashing. However, FieldDropdown still causes a layout when it is initialized.

Zoomed out: The red marks are all layouts

This is because of the way render is called in Field init, and then the dropdown modifies the text class and renders again. Because it changes the text class, it has a different cache key. So the initial render in field never hits a valid cache (it is always 0 because it isn't visible yet), and so keeps returning to the DOM for a measurement that will never succeed.

Hopefully we can remove the multiple renders, or change the way the class attributes are applied to field text so that we can stop trying to measure text that never gets rendered.

Removing the render in Field#init (not sure if that is the right answer) reduces workspace#refreshToolboxSelection from 190ms to 160ms, and removes the thrashing:

---

The next part that thrashes is at the end of the above image, in FlyoutButton.createDom. That one is actually pretty simple. Instead of using a caching method for getting the width, it always goes to the dom to measure. We can just change that from using getComputedTextLength with Blockly.Field.getCachedWidth.

---

In clearOldBlocks, we are spending a fair amount of time creating events that I do not think are needed. If we disable events before and reenable events after clearOldBlocks, we can save a fair bit of time (~10ms out of the total 33ms) from clearOldBlocks.

---

Also in clearOldBlocks, we are spending time clearing timeouts that don't actually exist. If we check for Blockly.Tooltip.showPid_ before calling clearTimeout, we can save almost 10ms off the total 33ms from clearOldBlocks.

---

All these together take out about 60ms from the 200ms total for workspace.refreshToolboxSelection on my machine.