scratchfoundation / scratch-flash

Open source version of the Scratch 2.0 project editor. This is the basis for the online and offline versions of Scratch found on the website.
https://scratch.mit.edu
GNU General Public License v2.0
1.33k stars 512 forks source link

Flash 30 performance changes #1396

Closed cwillisf closed 6 years ago

cwillisf commented 6 years ago

In an effort to mitigate Spectre & Meltdown attacks, Flash version 30 made a change to the getTimer() function: it's now very slow to call getTimer() repeatedly. Unfortunately, Scratch uses getTimer() quite a bit and this change had a huge impact on the performance of many Scratch projects.

This PR reduces the number of times we call getTimer() in two ways:

The new CachedTimer class wraps getTimer() with two calls. getFreshTimer() always calls getTimer() but caches the result; getCachedTimer() returns the most recent cached result. The clearCachedTimer() call clears the cache, forcing the next getCachedTimer() to act like getFreshTimer(). Nearly all calls to getTimer() have been replaced with CachedTimer.getCachedTimer(), except for those few which need a fresh value for timing checks mid-frame.

Unfortunately Scratch's main interpreter loop calls getTimer() after every block runs, and it needs a fresh value because it's checking if the interpreter loop has used up all of this frame's allowed work time. This PR reduces the frequency of these checks: it calculates an estimate for the number of blocks which ran in the last several "full" frames (frames which ran out of work time), and tries to check about 3 times during the "average" frame. In theory this may result in going over budget if a project runs many "cheap" blocks in one frame then many "expensive" blocks in the next frame, but checking 3 times per average frame should mitigate that and the statistics should correct themselves after a few frames.

I also removed some code related to tracking performance and framerate since that code was not being used and made calls to getTimer().

TheLogFather commented 6 years ago

I missed this, unfortunately...

Anyway, now that 461 is released, I notice that checking timer within non-refresh loops is broken, and I'd guess this is why.

This is bad if a project wants to do work for a set amount of time between each refresh.

Here's a simple example: https://scratch.mit.edu/projects/10125047/

Here's a more complex example: https://scratch.mit.edu/projects/47680290/ (–it's supposed to show progress every second)

TheLogFather commented 6 years ago

Instead of getTimer(), could the "timer" block use a "Date" object (subtracting the "Date" that was recorded on GF clicked, or from the most recent "reset timer")...? This makes it effectively based on the same mechanism as "days since 2000" (which is working fine).

TheLogFather commented 6 years ago

OK, after looking through the new code, I'm somewhat confused why it's not working...

It looks to me as though the "timer" block does use getFreshTimer, so should get an up-to-date value.

However, a simple test project with a single non-refresh custom block with a loop that adds the timer to a list a million times shows that the timer isn't getting updated... :/

TheLogFather commented 6 years ago

Ah, no, I take back my confusion above...

I see that app.runtime.timer does use interp.currentMSecs, so it's not surprising that the timer doesn't get updated correctly during non-refresh.

Could this be fixed...? (Is it possible to switch to using a "Date" for timer instead?)

BryceLTaylor commented 6 years ago

Running complex projects that heavily use the pen barely run and each frame takes forever to run.

Repro steps: Run this project: https://scratch.ly/projects/1000015568 (Original on production: https://scratch.mit.edu/projects/146120846)

Note: It barely runs on scratch.ly (Side note, it runs really well on 3.0)