[perf | trim] Remove extra class

[lifeart]

It seems we don't need to have extra class SingleNodeBounds for 2 cases

[NullVoxPopuli]

How does this affect perf? Is it a memory thing? Or something else?

I've been trying to get tracerbench setup, but no success yet

[lifeart]

I think it's likely memory thing, comparing to perf, creating one class instead of 2 in 10000 items is 1% faster.

jsbench

class A {
 	constructor(a,b,c) {
   this.a = a;
    this.b = b;
    this.c = c;
  }
}

class B {
  constructor(a,b,c) {
    this.a = a;
    this.b = b;
    this.c = c;
  }
}
var items = new Array(10000).fill(null);

items.map((e,index) => {
	return index % 2 === 0 ? new A(Math.random(), Math.random(), Math.random()) : new B(Math.random(), Math.random(), Math.random());
});

items.map((e,index) => {
	return index % 2 === 0 ? new A(Math.random(), Math.random(), Math.random()) : new A(Math.random(), Math.random(), Math.random());
});

[NullVoxPopuli]

Hm, impact is even greater on Firefox. Nice find!

[chancancode]

This change itself is fine but this is definitely not a valid way to benchmark this change. Please refrain from making perf changes from microbenchmarks like these alone

[victor-homyakov]

@lifeart @NullVoxPopuli @chancancode

I'm sorry, maybe it's too late. But I've analyzed the microbenchmark https://jsben.ch/EZXRT and I think it has some problems.

1. First of all, it is a microbenchmark and does not imitate the real use cases. There are two of them.

In the first case, we conditionally create either ConcreteBounds or SingleNodeBounds depending on some value named first. In this case, the impact on speed and memory depends on how often we get into the branch with SingleNodeBounds.

  __appendFragment(fragment: SimpleDocumentFragment): Bounds {
    // ...
    if (first) {
      let ret = new ConcreteBounds(this.element, first, fragment.lastChild!);
      // ...
    } else {
      // ...
      return new SingleNodeBounds(this.element, this.__appendComment(''));
    }
  }

In the second case, we always create SingleNodeBounds.

  appendDynamicNode(value: SimpleNode): void {
    // ...
    let bounds = new SingleNodeBounds(this.element, node);
    // ...
  }

The microbenchmark tries to imitate only the first case, and only when branch chances are 50/50. I have no information about the real-world applications but I doubt that they all have the same chances. Also, the microbenchmark completely ignores the second case. Ideally, @lifeart should have determined amounts of class constructor invocations and typical patterns (e.g. new ConcreteBounds() ... new SingleNodeBounds() ... new SingleNodeBounds() ... new ConcreteBounds() ... new SingleNodeBounds() ... new SingleNodeBounds()) and reproduce it in the benchmark.

2. The microbenchmark tries to find subtle differences in speed but does not reproduce all the subtle differences between classes under test. Any difference could completely change the results. The most obvious difference for me is the number of parameters passed to the constructor.

I've changed the code to be closer to the original classes using the compiled code from @glimmer/runtime/dist/prod/index.js. The fixed benchmark is located at https://jsben.ch/DyJmy.

Unfortunately, my modified benchmark shows opposite results, i.e. two original classes might be up to 20% faster than a proposed single class
:(

Chrome 120 on Apple M1:

Safari 17.2 on Apple M1:

Firefox 122 on Apple M1:

Anyway, what I want to say is:

• Such microbenchmarks should be thoroughly analyzed for errors and problems otherwise they will misguide you. One should have a lot of experience to write more or less correct microbenchmarks that could pass even a lightweight analysis.
• Microbenchmarks with a 1 or 2 percent change in speed should not be a reason to accept PR.
• Microbenchmarks with a 20% change in speed should be analyzed even more thoroughly :)
• Microbenchmarks should be run on every main JS engine that could be met in production, at least on Chrome Desktop, Firefox Desktop, Safari Desktop, Safari iOS, and Chrome Android. As you can see from the attached screenshots here and in [perf] replace Math.max usage with reduce and pure compare #1520 (comment), the results may vary.

[NullVoxPopuli]

on FireFox 124.0a1, Linux, Ryzen 9 7900x

which shows... the opposite.
but in chrome:

opposite again.

maybe it's negligible? idk
I wonder what an updated js-framework-benchmark shows

[victor-homyakov]

maybe it's negligible?

Agree. 3-4% in the synthetic benchmark is negligible.

[chancancode]

maybe it's negligible?

Agree. 3-4% in the synthetic benchmark is negligible.

Yes, as I said in the comment when merging this, the benchmarks are not valid and this is not a place where performance likely would matter.

I merged it because the extra class does seem a bit superfluous, used sparingly and didn’t help with readability enough to warrant it existing.

But I kept trying to say here and elsewhere, making these kind of changes based on narrow microbenchmarks are no good, and even synthetic benchmarks like js-framework-benchmark is not representative of real world patterns.

[NullVoxPopuli]

reducing amount of code to maintain is still nice, so that's a win

making these kind of changes based on narrow microbenchmarks are no good

agreed

js-framework-benchmark is not representative of real world patterns

big disagree. This is a decision by which folks judge our ecosystem and framework, and at the very least we need to not trip and fall flat on our face <3

thankfully, there is a ton of room for improvement!!!
Like, @chancancode and I are pairing on cleaning up the VM: #1547
and less code + fewer features = faster code! 🎉