Skip to content

Should updateProperties iterate properties instead of testing in? #329

Open
@cwillisf

Description

@cwillisf
Contributor

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?

Activity

added this to the Backlog milestone on Aug 7, 2018
changed the title [-]Iterate properties instead of testing `in`?[/-] [+]Should `updateProperties` iterate properties instead of testing `in`?[/+] on Aug 7, 2018
mzgoddard

mzgoddard commented on Aug 13, 2018

@mzgoddard
Contributor

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @mzgoddard@cwillisf

        Issue actions

          Should `updateProperties` iterate properties instead of testing `in`? · Issue #329 · scratchfoundation/scratch-render