fix(apiWatch): deep watch should support symbols, performance tweaks

[dsseng]

Increased perf of traverse using for (let i...) {} instead of for...in/for...of. Fixed traverse so it will track symbols and add a test case for symbol.

[yyx990803]

The cost here is almost the same because it involves spreading the iterator into a plain array. The array spread can often be deceivingly expensive. Not really worth it IMO.

[dsseng]

I tested set iterating in some browsers:
Chrome 79.0.3941.4 (64 bit, V8 7.9.293)

Firefox 70.0 (64 bit) (мс stands for millisecond in Russian, таймер закончился = timer ended)

I ran tests in Node too, but I had to call functions separately since console gets clogged but I don't want to change the task:
Node v12.8.0 (64 bit):

I loaded the same start code:

Small set:

100 elements:

Now, 10k elements:

So, spread method gives better performance than for...of, but (why?) was worse in Chrome and Node with 100-element set

Code:

function forOfSet (val) {
  console.time('for...of')
  console.groupCollapsed('data')
  for (const x of val) {
    console.log(x)
  }
  console.groupEnd()
  console.timeEnd('for...of')
}

function forSpreadSet (val) {
  console.time('for with spread')
  console.groupCollapsed('data')
  const arr = [...val]
  for (let i = 0; i < arr.length; i++) {
    console.log(arr[i])
  }
  console.groupEnd()
  console.timeEnd('for with spread')
}

const set1 = new Set([1, 2, 3])
const set2 = new Set(Array(100).fill(null).map((_, i) => i))
const set3 = new Set(Array(10000).fill(null).map((_, i) => i))

forOfSet(set1)
forSpreadSet(set1)

forOfSet(set2)
forSpreadSet(set2)

forOfSet(set3)
forSpreadSet(set3)

[MarkVasile]

On my side I see no significant difference even with a million items in the set. I've looked at both timing and memory performance indicators, refreshing the page for each particular method (so run test with spread, change code to for..of and refresh, a few times over).

Chrome Version 79.0.3945.88 (Official Build) (64-bit) on MacOS Catalina (10.14.6) (spec 2.4GHz i5, 16GB 2133MHz LPDDR3) by launching chrome with:

/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --enable-precise-memory-info --js-flags="--trace-opt --trace-deopt"

function forOfSet(val, index) {
  console.time('for set' + index)
  const acc = []
  // const arr = [...val]
  for (const x of val) acc.push(x)
  // for (let i=0; i<arr.length; i++) acc.push(arr[i])
  console.timeEnd('for set' + index)
  console.log('memory: total, used, sizeLimit:', window.performance.memory.totalJSHeapSize, window.performance.memory.usedJSHeapSize, window.performance.memory.jsHeapSizeLimit)
  console.log(acc)
}

const set1 = new Set([1,2,3])
const set2 = new Set(Array(100).fill(null).map((_,i) => i))
const set3 = new Set(Array(1000).fill(null).map((_,i) => i))
const set4 = new Set(Array(1000000).fill(null).map((_,i) => i))

// forOfSet(set1, 1)
// forOfSet(set2, 2)
// forOfSet(set3, 3)
forOfSet(set4, 4)

The timing varies with each refresh in a range of 30 to 45 ms, regardless of the loop method being used.

[yyx990803]

Not sure if this should be the case here. This would iterate over non-enumerable properties too.

[yyx990803]

I think the technically correct implementation here is using getOwnPropertyDescriptors and then traverse enumerable properties. But I don't know if it's worth it. May want a benchmark to evaluate the perf cost compared to a simple for...in.

[dsseng]

I can try to make a benchmark just as in upper comment :).

[dsseng]

But for...in seems not to list symbols.

[dsseng]

Benchmark (anything as the first comment):
Chrome:

FF:

Node:

Results of this benchmark are questionable, and I think both of these should be validated in e.g. a CI env, since my CPU frequency and load isn't really stable.

Code:

function forInObject (val) {
  console.time('for...in')
  console.groupCollapsed('data')
  for (const x in val) {
    console.log(x)
  }
  console.groupEnd()
  console.timeEnd('for...in')
}

function forSpreadSet (val) {
  console.time('for with spread keys')
  console.groupCollapsed('data')
  const arr = [...Object.keys(val)]
  for (let i = 0; i < arr.length; i++) {
    console.log(arr[i])
  }
  console.groupEnd()
  console.timeEnd('for with spread keys')
}

const obj1 = { '1': 1, '2': 2, '3': 3 }
const obj2 = {}
for (let i = 0; i < 100; i++) {
  obj2[i] = i
}
const obj3 = {}
for (let i = 0; i < 10000; i++) {
  obj3[i] = i
}

forInObject(obj1)
forSpreadSet(obj1)

forInObject(obj2)
forSpreadSet(obj2)

forInObject(obj3)
forSpreadSet(obj3)

[panstromek]

I just stumbled upon this thread randomly and I need to say that I would stay away from microoptimizations like this because they have very noisy and unpredictable results. Since this is not the first time I see things like this here, I have a few things to note:

1. Browsers have multiple ways to run the same code and the performance of that code literally depends on everything else even if it seems completely irrelevant. From the size of the source code to heat of your CPU.
2. Browsers optimize the code on the fly based on how it's used, so if you just run it million times in a row, it will probably optimize it a lot which biases the results towards optimized code.
3. The problem is - most code is NOT optimized, because doing so is slow and browsers usually prioritize running the code sooner, so they will first interpret it and only optimize it when it is run multiple times. If you want to optimize something, you can't rely on browser optimizing it.
4. Browsers constantly change how this all works in order to improve. They try to balance many tradeoffs. This means that your benchmarks may already be outdated and will almost certainly be outdated in the future
5. On top of that, many bundlers actually change that code significantly during bundling. For example, in lot of common configs, Babel just replaces the spread operator with something equivalent which works in more browsers but in many cases it's slower.

This all combined means, that if you want to reliably optimize something in JS, you can only make very few assumptions and only do things that are reliably better.

1. In most cases, this means just simply doing less work - iterating over less things, process less data etc. Things that the browser cannot figure out on its own because it doesn't understand the intent behind your code
2. Using JS features that are likely to be optimized already - it has been highlighted by browser developers that using things like Array.forEach is good, because these are built into the engine and they are already optimized upfront. And even if they are not optimized at the moment, they will likely be better optmized in the future.
3. Avoiding patterns that have "Slow semantics" as I call it. Those are things with semantic meaning that already implies doing more work and browsers need to optimize it to make it fast.

Last point is especially a problem with this PR - for example, instead of iterating over Map with builtin .forEach, it first allocates new array, spread all elements from the Map into it and iterate on that - iterating 2 times instead of 1. That is simply just more work to do SEMANTICALLY.

Now it's improtant to note - that might actually be faster at the moment. It is possible that you hit some weird edge case that browsers didn't cover. But because it is semantically more work, you can't rely that this will be the case in the future. Browsers may catch up in the next release and optimize the builtin better.

Manual iteration can also be a bit sneaky. Even though it is faster than functional iteration in lower level languages, in JavaScript it gives the developer too much freedom, so the engine first need to "figure out" that it actually is manual iteration. If you ask for an element on index with thing[i], engine needs to typecheck the thing and check that index doesn't go out of bounds. It does this check by default, but it will optimize it out when it can infer this from loop control. It needs to do some work to figure that out, though, because JavaScript allows loop control to be almost any code.

If you use .forEach in modern browsers, you can assume that browser already has version of that function without additional type&bounds checks, because it has loop code under control and doesn't need to figure it out from your code. The same thing applies to for .. of and for .. in. These constructs are all more restricted than traditional index-based loops, which gives the browsers more opportunities for optimization. Again - this can be slower at the moment, but has more potential to be faster in the future. This applies especially for interpreting the code, which is done by default on page load and doesn't involve almost any optimization.

That's why optimizations like this that just "do the same thing but in a slighly different syntax" are usually not worth it.

Anyway, this ended up being much longer than I first expected, but I hope it gives some useful information to consider 😉 I don't mean this to be overly negative, it's great to work on performance. I just wanted to clarify some concerns, because I myself lost a lot of time optimizing JS code and it is often very counter intuitive and you can't just make straightforward conclusions.