scratchfoundation / scratch-render

WebGL-based rendering engine for Scratch 3.0
BSD 3-Clause "New" or "Revised" License
260 stars 333 forks source link

Should `updateProperties` iterate properties instead of testing `in`? #329

Open cwillisf opened 6 years ago

cwillisf commented 6 years ago

Currently, the updateProperties function works like this:

updateProperties (properties) {
    let dirty = false;
    if ('position' in properties && (...)) { ... }
    if ('direction' in properties && (...)) { ... }
    if ('scale' in properties && (...)) { ... }
    if ('visible' in properties) { ... }

There's also an if (effectName in properties) check for each effect.

Since updateProperties is called so frequently, it's possible that doing so many in checks is unnecessarily expensive. We could instead iterate the properties of properties (heh) and act on what we find, for example.

It's hard to say for sure whether this would improve performance without benchmarking, though: the properties object tends to not have very many entries per call, but does that mean that the in checks are cheap or does it mean that iterating is an even better idea? Is the time spent handling the in checks even significant to begin with?

mzgoddard commented 6 years ago

The standard fast test for these kind of operations with static keys is typeof properties.position !== 'undefined'. Testing with if statements and the typeof undefined is likely the best cross-browser approach for static keys.

Here is a benchmark with a few ways I think this could be written: https://esbench.com/bench/5b6c5c43f2949800a0f61f30

Variable keys like if (effectName in properties) seems to already be pretty good. A typeof check in a function with a static key will beat it.

Benchmark for variable keys: https://esbench.com/bench/5b71d75cf2949800a0f61f4a (An exists function in Safari does really well.)