Delay loading and rendering gui #4800

mzgoddard · 2019-05-07T18:06:04Z

Resolves

Faster project load time when loading a project by id.

Proposed Changes

Related #4722.

Create the scratch renderer earlier in the GUI container/HOC stack
Delay evaluating and rendering the GUI component until after project assets have been fetched

Reason for Changes

When loading a project by id scratch needs to load a data file and then load assets referenced by that data file. Since these requests work asynchronously outside the javascript event loop, they make events to announce to javascript when they have completed some work. GUI does a lot of work that first blocks the data request from being sent and then blocks the response events from being emitted. Delaying this GUI work until the data and assets have loaded significantly improves load time.

This PR follows #4722 by delaying some work. In this case it creates the scratch renderer earlier and delays all other GUI work until after the project loads. #4722 by comparison optionally breaks up GUI into smaller

Further Work

This idea can be taken further. A future change could move creating the scratch storage out of the react + redux stack. Loading the project data only requires the Storage instance. Loading the project assets requires the Renderer, AudioEngine and VirtualMachine.

TODO

Loading screen

The loading screens are currently embedded in the components that are in a sense being loaded. Needing to render the component to render its loading screen makes it happen too late when combined with this PR. How should we solve this?

When can the GUI be rendered

Currently it is best that most of the GUI not be evaluated or rendered until after the asset requests are made. Currently there is no state available know when this has happened. To best know when the GUI can be rendered we need this state. Alternatively we can render the GUI when the project has finished loading (as this PR currently does).

Loading a second project will unmount the GUI

How does the container know that we have already rendered the GUI before?

I think the best answer would be two "loading screens" wrapping the container or a higher component. One that renders while there is a renderer and isLoading is true and then never render again. And a second that always renders while isLoading is true.

The easy solution here would probably be to unmount the GUI and render a loading screen here.

Should Renderer and AudioEngine be initialized in React

This question is "too big" for this PR. But I find myself asking it since its moving Renderer's creation point. Since the VM is in the redux store, maybe Renderer and AudioEngine should be initialized by a redux middleware.

Benchmarks

Benchmark methodology source

diff --git a/src/lib/vm-manager-hoc.jsx b/src/lib/vm-manager-hoc.jsx
index fd7e3ed3..2599c8b6 100644
--- a/src/lib/vm-manager-hoc.jsx
+++ b/src/lib/vm-manager-hoc.jsx
@@ -55,6 +55,8 @@ const vmManagerHOC = function (WrappedComponent) {
             return this.props.vm.loadProject(this.props.projectData)
                 .then(() => {
                     this.props.onLoadedProject(this.props.loadingState, this.props.canSave);
+                    window.LOAD_END = Date.now();
+                    console.log('load', window.LOAD_END - window.LOAD_START);
                     // Wrap in a setTimeout because skin loading in
                     // the renderer can be async.
                     setTimeout(() => this.props.onSetProjectUnchanged());
diff --git a/src/playground/index.jsx b/src/playground/index.jsx
index dcf6fa18..674bce7f 100644
--- a/src/playground/index.jsx
+++ b/src/playground/index.jsx
@@ -1,3 +1,5 @@
+import './start-time';
+
 // Polyfills
 import 'es6-object-assign/auto';
 import 'core-js/fn/array/includes';
diff --git a/src/playground/start-time.js b/src/playground/start-time.js
new file mode 100644
index 00000000..999c795f
--- /dev/null
+++ b/src/playground/start-time.js
@@ -0,0 +1 @@
+window.LOAD_START = Date.now();

environment	device	project id	develop	delay-gui	difference	change
without devtools	chrome	173918262	2229	1556	673	30.21%
devtools and no cache	chrome	173918262	3429	2313	1116	32.55%

Performance is a little better in this PR, delay-gui, when compared to the dynamic-stage PR #4722. Primarily this is because delay-gui performs its delay at an earlier time than the other PR. Optimistically that will load faster by making the async requests earlier. A similar change could be made for #4722 to add an earlier delay breakpoint.

Detailed Chart

environment	device	project id	branch	run 1	run 2	run 3	run 4	run 5	average	standard deviation
without devtools	chrome	173918262	develop	2286	2231	2272	2179	2177	2229	51
without devtools	chrome	173918262	dynamic-stage	1677	1670	1782	1658	1598	1677	66
without devtools	chrome	173918262	delay-gui	1522	1537	1622	1561	1536	1556	40
devtools + disable cache	chrome	173918262	develop	3275	3544	3459	3419	3450	3429	98
devtools + disable cache	chrome	173918262	dynamic-stage	2478	2335	2480	2310		2401	91
devtools + disable cache	chrome	173918262	delay-gui	2398	2359	2146	2280	2382	2313	104

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

Chrome
Firefox
Safari

Windows

Chrome
Firefox
Edge

Chromebook

Chrome

iPad

Safari

Android Tablet

Chrome

paulkaplan · 2019-05-08T13:08:07Z

src/containers/gui.jsx

+        // Use setTimeout. Do not use requestAnimationFrame or a resolved
+        // Promise. We want this work delayed until after the data request is
+        // made.
+        setTimeout(() => {


@mzgoddard I think this introduces a non-deterministic race condition where it is possible for the assets to be loaded before the VM has a renderer. (You can confirm this by putting in a long timeout time at the end of this). I noticed because I was getting errors like "no rendering module present" sometimes when testing.

I implemented the load time testing you used (thanks for showing how to do that!) and it at first look it doesn't seem to make a difference (still looks like approximately 2000ms -> ~1500ms), so maybe for getting this in we could just inline the renderer creation before the project load?

Crud. I fixed this yesterday and forgot to push the change.

The setTimeout doesn't create a race condition. The race condition is effectively in render() since its what blocks the existing renderer creation. We need to test for this in the if block before rendering the GUI.

Here's the fixed logic.

if ( !fontsLoaded || fetchingProject || this.props.vm.renderer && isLoading ) { // TODO: Render a loading screen. return null; }

I'm going to take this a step further and ensure the renderer by this point.

paulkaplan · 2019-05-08T13:18:21Z

src/containers/gui.jsx

+        // as with "Load from your computer").
+        if (!fontsLoaded || fetchingProject || isLoading) {
+            // TODO: Render a loading screen.
+            return null;


@mzgoddard Returning a <Loader /> component here seems like a reasonable thing to do.

As a side effect, I think this gets us to a "first meaningful paint" faster, because we would have a loader being rendered without all the rest of the GUI, so not only does the time to "seeing the full GUI" go down, but so does "time to seeing the loading screen instead of a blank page".

Cool.

This will cause a screen flash. When the <Loader /> is replaced by the Loader in GUI the animation will restart because it is a new DOM element, and the CSS animation will be a different instance. Fixing this flash will be a next step perhaps.

Or. Hmm. I'm not sure that will happen. It will probably normally not happen since that be the same render when loading is false.

Uhm.

paulkaplan · 2019-05-08T13:28:52Z

src/containers/gui.jsx

@@ -102,6 +139,17 @@ class GUI extends React.Component {
            loadingStateVisible,
            ...componentProps
        } = this.props;
+
+        // TODO: Determine if we finished loading previously. In which case we


I would be fine with adding a flag to this container for the previous load (could set it in componentDidUpdate above?). In theory it is actually fine to just re-mount the whole GUI (we do that sometimes for other things, like changing languages), but it would probably be a wasted render.

Cool. Uh I think it has to be in both Update and Mount. At least to be safe, we could in theory be done fetching before we render this component.

paulkaplan · 2019-05-08T13:30:59Z

@mzgoddard this is looking great! A few comments. I think the tests are probably failing due to the lack of loading screen, might be fixed after that?

- Create the scratch renderer earlier in the GUI container/HOC stack - Delay loading and rendering GUI component until after project assets have been fetched

paulkaplan

Code LGTM, do you think this is ready to merge?

mzgoddard added pr - needs review performance labels May 7, 2019

paulkaplan reviewed May 8, 2019

View reviewed changes

paulkaplan added pr - changes requested and removed pr - needs review labels May 8, 2019

mzgoddard force-pushed the delay-gui branch from bb862da to cae1286 Compare May 8, 2019 19:56

delay loading and rendering gui

bec6223

- Create the scratch renderer earlier in the GUI container/HOC stack - Delay loading and rendering GUI component until after project assets have been fetched

mzgoddard force-pushed the delay-gui branch from cae1286 to bec6223 Compare May 8, 2019 20:58

thisandagain assigned mzgoddard May 13, 2019

thisandagain added this to the May 2019 milestone May 13, 2019

paulkaplan added pr - good to merge and removed pr - changes requested labels May 23, 2019

paulkaplan approved these changes May 23, 2019

View reviewed changes

paulkaplan merged commit d342b82 into scratchfoundation:develop May 23, 2019

This was referenced May 29, 2019

[Develop] Release/2019-05-29 scratchfoundation/scratch-www#3021

Merged

[Master] Release/2019-05-29 scratchfoundation/scratch-www#3022

Merged

This was referenced May 30, 2019

Revert "Delay loading and rendering gui" #4873

Merged

Revisit initial load performance PR that was reverted #4876

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Delay loading and rendering gui #4800

Delay loading and rendering gui #4800

Uh oh!

mzgoddard commented May 7, 2019

Uh oh!

paulkaplan May 8, 2019

Uh oh!

mzgoddard May 8, 2019

Uh oh!

paulkaplan May 8, 2019

Uh oh!

mzgoddard May 8, 2019 •

edited

Loading

Uh oh!

mzgoddard May 8, 2019

Uh oh!

paulkaplan May 8, 2019

Uh oh!

mzgoddard May 8, 2019

Uh oh!

paulkaplan commented May 8, 2019 •

edited

Loading

Uh oh!

paulkaplan left a comment

Uh oh!

Uh oh!

Delay loading and rendering gui #4800

Delay loading and rendering gui #4800

Uh oh!

Conversation

mzgoddard commented May 7, 2019

Resolves

Proposed Changes

Reason for Changes

Further Work

TODO

Benchmarks

Browser Coverage

Uh oh!

paulkaplan May 8, 2019

Choose a reason for hiding this comment

Uh oh!

mzgoddard May 8, 2019

Choose a reason for hiding this comment

Uh oh!

paulkaplan May 8, 2019

Choose a reason for hiding this comment

Uh oh!

mzgoddard May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzgoddard May 8, 2019

Choose a reason for hiding this comment

Uh oh!

paulkaplan May 8, 2019

Choose a reason for hiding this comment

Uh oh!

mzgoddard May 8, 2019

Choose a reason for hiding this comment

Uh oh!

paulkaplan commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzgoddard May 8, 2019 •

edited

Loading

paulkaplan commented May 8, 2019 •

edited

Loading