Scratch-gui: Performance issue when switch tabs to blocks

[gengshenghong]

Expected Behavior

Should be much more fluent.

Actual Behavior

A little slow.

Steps to Reproduce

1. Open https://llk.github.io/scratch-gui/develop/
2. Switch to "Costumes" or "Sounds" tabs
3. Switch back to "Blocks" tab, it takes about 1 seconds (500ms? whatever, not that fast...) to display the Blocks tab

It is very fast several days ago, before the "flyout scroll" function is merged. I guess below commit is related:
https://github.com/LLK/scratch-blocks/pull/1108
https://github.com/LLK/scratch-blocks/pull/1113

Operating System and Browser

Ubuntu 14.04, Chrome.

[griffpatch]

I don't suppose there is a way to keep the flyout panel resident rather
than disposing and re-creating it each time the tabs are toggled?
Blockly appears to take quite some time in the destroy and especially the
re-creation of the blocks palette.

On 9 October 2017 at 07:58, Shenghong [e-mail] wrote:

Expected Behavior

Should be much more fluent.
Actual Behavior

A little slow.
Steps to Reproduce

1. Open https://llk.github.io/scratch-gui/develop/
2. Switch to "Costumes" or "Sounds" tabs
3. Switch back to "Blocks" tab, it takes about 1 seconds (500ms?
   whatever, not that fast...) to display the Blocks tab

It is very fast several days ago, before the "flyout scroll" function is
merged. I guess below commit is related:
LLK/scratch-blocks#1108 https://github.com/LLK/scratch-blocks/pull/1108
LLK/scratch-blocks#1113 https://github.com/LLK/scratch-blocks/pull/1113
Operating System and Browser

Ubuntu 14.04, Chrome.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748, or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvn_KjmE05VIWf4V9n9TR0X5J0XoCks5sqcQxgaJpZM4PyEJF
.

[thisandagain]

/cc @rschamp @picklesrus @paulkaplan

[griffpatch]

I have tracked down a massive performance issue that explains why sometimes
things seem very very slow and it's all down to the use of 'variables'.
If you don't have any variables declared then you can switch from sprite to
sprite pretty fast, but if you simply add a few variable reporter blocks
onto an empty script page in a new project then the whole thing slows down
exponentially.

Steps to replicate:

• Start new Empty Project
• Add a second sprite
• On the first sprite create a new variable
• Drag the variable reporter onto the script panel of the first sprite.

Now with a single reporter on the script panel it takes my chrome browser
on a core i7 pc around 1.5 seconds to switch from the second sprite back to
the first sprite script pane.

The next step is to add a second reporter and then take a similar timing of
switching sprites.

Here is a table showing the number of reports vs the time it takes to
switch sprites:

1 reporter = 1.5 seconds
2 reporters = 2 seconds
4 reporters = 3 seconds
8 reporters = 5 seconds
16 reportes = 10 seconds
32 reporters = 25 seconds
48 reporters = 43 seconds

Here is a screenshot of 48 reporters that takes 43 seconds... Note it
doesn't matter if they are nested within other blocks or not... it still
takes this long and appears to be only caused by variable reporter blocks

A similar test with built in reporters is very quick (here are 80 x
position reporters, and it takes 1 second to switch sprites)

Hope this revelation helps.

Griffpatch

On 9 October 2017 at 18:32, Andrew Sliwinski [e-mail]
wrote:

/cc @rschamp https://github.com/rschamp @picklesrus
https://github.com/picklesrus @paulkaplan
https://github.com/paulkaplan

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748#issuecomment-335226678,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvjVkHa0mUcGHaB3PB4HMe_dZVUmAks5sqliegaJpZM4PyEJF
.

[griffpatch]

Should this be moved to another issue? And which project should it go under?

[gengshenghong]

i think that is another issue. you may submit to gui project first to track.

regarding this issue, it is introduced in recent commit of flyout scroll feature. it was very fast days ago when no scroll feature.

[paulkaplan]

@griffpatch yeah the variable rerender was definitely a huge problem. It was actually just solved with https://github.com/LLK/scratch-blocks/pull/1122 which was just merged. However that doesn't solve the "empty workspace" switching time of around 500ms.

[griffpatch]

Look forward to seeing whether this fix does address the issue... Not sure
it explains the different between variable reporters and normal reporters?

32 x-position reporter blocks take 1 second to switch tabs, whereas 32
variable reporter blocks takes 25 seconds.

On 11 October 2017 at 14:56, Paul Kaplan [e-mail] wrote:

@griffpatch https://github.com/griffpatch yeah the variable rerender
was definitely a huge problem. It was actually just solved with
LLK/scratch-blocks#1122 https://github.com/LLK/scratch-blocks/pull/1122
which was just merged. However that doesn't solve the "empty workspace"
switching time of around 500ms.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748#issuecomment-335817946,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvpTr6FPocQiE4c1XP9V7eLK7IKtWks5srMj5gaJpZM4PyEJF
.

[paulkaplan]

@griffpatch I believe develop should now be deployed with the fix for the variable reporter delay.

[griffpatch]

Would that be on https://llk.github.io/scratch-gui/ - if so then it's not
fixed. Still takes 12 seconds to swap to a script page that just contains
32 "variable reporter" blocks (and nothing else). It's very quick for other
types of blocks... just those variable reporters.

On 11 October 2017 at 18:48, Paul Kaplan [e-mail] wrote:

@griffpatch https://github.com/griffpatch I believe develop should now
be deployed with the fix for the variable reporter delay.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748#issuecomment-335892909,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvr_PQ8rxqDUvjnAmC8qMmhLDFIkPks5srP-FgaJpZM4PyEJF
.

[Kenny2github]

@griffpatch the develop branch is at https://llk.github.io/scratch-gui/develop - the master branch is at https://llk.github.io/scratch-gui

[griffpatch]

Right! I tested again on that branch and you are quite right that the delay
is much reduced.
You might want to enable the same optimisation on using the right click
'duplicate' option as this is still very slow for block stacks that contain
variable reporter blocks.

Out of curiosity though, is this fix perhaps hiding the issue? Note how I
compare the speeds of normal reporter blocks (like x position) with the
variable reporter blocks... Why are variable reporter blocks causing so
much issue?

I'd love to help out, but developing on windows I really struggle to work
with the Blocky and closure-library, etc... they don't want to compile.

On 12 October 2017 at 08:43, Ken [e-mail] wrote:

@griffpatch https://github.com/griffpatch the develop branch is at
https://llk.github.io/scratch-gui/develop - the master branch is at
https://llk.github.io/scratch-gui

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748#issuecomment-336047848,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvsqsVzfov3k2F0PtN-gbTSLCqBwFks5srcMugaJpZM4PyEJF
.

[griffpatch]

Ok, I can see that this is the same issue, that the slowness is due to
every call to Blockly.WorkspaceSvg.createVariable refreshes the toolbox
(this.refreshToolboxSelection).
Adding the same fix to block duplication will fix this one too.

On 12 October 2017 at 09:06, Andrew Griffin [e-mail] wrote:

Right! I tested again on that branch and you are quite right that the
delay is much reduced.
You might want to enable the same optimisation on using the right click
'duplicate' option as this is still very slow for block stacks that contain
variable reporter blocks.

Out of curiosity though, is this fix perhaps hiding the issue? Note how I
compare the speeds of normal reporter blocks (like x position) with the
variable reporter blocks... Why are variable reporter blocks causing so
much issue?

I'd love to help out, but developing on windows I really struggle to work
with the Blocky and closure-library, etc... they don't want to compile.

On 12 October 2017 at 08:43, Ken [e-mail] wrote:

@griffpatch https://github.com/griffpatch the develop branch is at
https://llk.github.io/scratch-gui/develop - the master branch is at
https://llk.github.io/scratch-gui

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLK/scratch-gui/issues/748#issuecomment-336047848,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGbNvsqsVzfov3k2F0PtN-gbTSLCqBwFks5srcMugaJpZM4PyEJF
.

[paulkaplan]

Yup, that is the cause. And interesting about duplicate, I'll file an issue for that over on scratch-blocks. Thanks!

[rachel-fenichel]

@griffpatch we're tracking flyout/toolbox performance over in scratch-blocks: https://github.com/LLK/scratch-blocks/issues/879

And here's the bug to track creation of blocks with variables: https://github.com/LLK/scratch-blocks/issues/887

Basically, Blockly only eats the performance cost as the flyout is opened. Scratch has the flyout always open, so updating the toolbox is way more expensive. Variables are especially egregious offenders, but in general there are changes we can make to the flyout to make it faster.

Swapping the flyout to have all of the blocks in one long list, with categories doing anchor scrolling between them, also exposed these problems a lot more. When it was only ~5-15 blocks per category the layout thrashing was less noticeable.

[gengshenghong]

FYI. Just doing some test and it will be much much slower (about 5+ seconds!) when more blocks (~30 blocks appended) are added (example: if we have some extensions), which is bad user experience. In scratch2.0, there are lots of extensions with many blocks and no performance issues. So, this issue will be critical when extensions are loaded.

[gengshenghong]

If I added a log below in core/toolbox.js in scratch-blocks:

Blockly.Toolbox.prototype.refreshSelection = function() {
    console.log("refresh...");
  this.showAll_();
};

When switch from costumes to blocks tab, the log will be printed 4 times in console!!! It seems like very bad behavior as it will invoke showAll (construct the flyout contents) 4 times...Not sure about the other logic about why it will be invoked 4 times.

[paulkaplan]

@gengshenghong good find! I also checked sprite/sprite changes, those call it 3 times. It looks like two of them are being called because of the way the bulkUpdating change is turned off and then on again, causing a refresh, in both workspace#clear and xml#domToWorkspace. Those should probably be combined into a single scratch-blocks call. I'll file that as an issue on scratch-blocks.

The other two are because we previously needed to manually call refreshSelection from the GUI, but no longer need to. I'll try to remove those unnecessary calls now.

Very good find, should reduce switching times by a factor of 3 or 4!

[picklesrus]

A little late to the party, but I'm back to working on this. Yeah, it seems the bulkUpdate change fixed some cases, but made other things worse.

There's actually a slight behavior change it brings up as well. workspace.clear() now updates the toolbox and therefore deletes any variables from the data category. The variable behavior change seems like it would go unnoticed but I wonder if this might cause bugs with the extension blocks too?

Anyway, on the performance side switching sprites calls workspace.clear() which after the bulkUpdate change, now refreshes the toolbox even though it doesn't need to. That probably adds back a decent amount of the gain we got from it while loading from xml.
I think some parts of that change need to be backed out. We need different signals for "the workspace might have changed size" and "the toolbox contents may have changed". Still investigating though.

[rachel-fenichel]

Yeah, the bulkUpdate change looks like it wasn't quite what we want. It's forcing toolbox refreshes every time bulk update is turned off, rather than only if a refresh was requested during the bulk update.

[paulkaplan]

@picklesrus yeah that is good summary of what is going on. Maybe we should assemble a doc mapping out when updates are requested externally, what kind of updates are needed, and when updates are needed internally to get a better idea of how to manage these things. I can start putting something together.

[picklesrus]

@paulkaplan sounds great. I'm especially interested in the external part since I don't have as much visibility there. I'm going to wrangle some code so that there is a mechanism to add/remove variable blocks, etc without immediately updating the toolbox.

[picklesrus]

Update for @paulkaplan so we don't conflict. My plan is to

1. submit a small change to stop refreshing the toolbox when adding variable blocks where the variable already is in the variable map.
2. revert the bulk update change
3. add a better mechanism for bulk updates that specifically targets the toolbox.

Status on that plan:
My plan is for 1 & 2 to be done tomorrow, though I'm wrangling a unittest that has never been run on scratch-blocks. 3 will take a little more time (a day or two) but should be straightforward. Folks might notice the slowness for a day or 2 if they're looking at scratch-gui/develop, but I think it is worth it since the bulk update change makes seeing how to do the right thing harder.

[paulkaplan]

Going to close this as the behavior is now stable for projects with and without variables.