scratchfoundation / scratch-vm

Virtual Machine used to represent, run, and maintain the state of programs for Scratch 3.0
http://scratchfoundation.github.io/scratch-vm/
BSD 3-Clause "New" or "Revised" License
1.21k stars 1.51k forks source link

Investigate performance issues in "Bouncy Heros" projects #741

Open thisandagain opened 6 years ago

thisandagain commented 6 years ago

Expected Behavior

Actual Behavior

Steps to Reproduce

https://scratch.mit.edu/projects/173918262/

mzgoddard commented 6 years ago

Bouncy Heros shows some similar problems like Stacky Build (#740) in execute and stepThreads. It also has regular frame stutter (Stacky Build has a little of this) where getProdcedureParamName and getProcedureDefinition spend a lot of time building up the info to start a procedure.

execute and stepThreads

Bouncy Heros shares the same problems in execute and stepThreads described in https://github.com/LLK/scratch-vm/issues/740#issuecomment-341288496.

stamp

Stamping is spending a lot of time waiting for the GPU to be able to return pixel data through readPixels to set an array buffer of a canvas to draw on a final pen skin canvas. The best way to improve this would be to replace the readPixels and canvas drawing with render buffers to keep all this work on the GPU side. readPixels will always be costly since it needs to stop the rendering pipeline and busy wait for the GPU to return information.

getProcedureParamName and getProcedureDefinition

These two functions loop over potentially all of the blocks in a Blocks instance. These functions use for (const id in this._blocks) which is syntactic sugar for iterating an Object.keys array of the blocks dictionary. Creating the underlying array of keys through for-in or Object.keys is costly to do every time a procedure is starting. This can be greatly improved by maintaining an array of values. The array could be directly maintained in createBlock or deleteBlock or marked dirty and recreated on the first use in getProcedureParamName or getProcedureDefinition.

Another good step would be caching the end result like in @griffpatch's optimization work. https://github.com/LLK/scratch-vm/issues/740#issuecomment-341359004

This isn't to say using for-in is a bad practice. for-in is great for objects that aren't known ahead of time in some fashion. Its more clear and readable for maintaining. In the smaller set of areas like here that is called very frequently on stable data, looping over a cached array instead of the object dictionary provides a stronger benefit to the application than the more readable and maintainable code.

Minor GC

The scratch-vm creates a lot of very temporary objects leading to regular Minor GC (Garbage Collection) events. Reducing the number of regularly created objects will reduce the amount of time JavaScript needs to use to do this and more importantly how regularly it needs to. Right now about every frame in Chrome is running Minor GC. The memory usage of this project in the player only scratch-gui example jumps between 32MB and 50MB.

Places like execute and getInputs instead of creating new objects every time, either reusing a single static instance (execute's util object) or store on a passed in object (getInputs's returned object) will help reduce the regular amount of time creating and collecting objects.

Some Numbers (MBP 13" 2013)

Percent of time spent inside a function from a 5 second sample of runtime.

operations % of 5 seconds
execute 24.4
getProcedureParamNames 9.8
stepThreads 7.2
getProcedureDefinition 7.2
Minor GC 4.4
indexOf 4.1
readPixels 3.9