Explore performance 3

[NullVoxPopuli]

Related:

• Explore performance #1596
• Explore Performance 2 #1597

The goal of this PR is to try to see if we can undo the perf losses reported by

• Rendering performance regression between 0.84.3 and 0.85.1? #1590

[github-actions]

duration phase estimated improvement -775ms [-863ms to -681ms] OR -5.23% [-5.83% to -4.6%]
renderEnd phase estimated improvement -4ms [-6ms to -2ms] OR -6.05% [-8.57% to -3.32%]
render1000Items1End phase estimated improvement -82ms [-91ms to -72ms] OR -7.02% [-7.86% to -6.21%]
clearItems1End phase no difference [-3ms to 3ms]
render1000Items2End phase estimated improvement -53ms [-63ms to -43ms] OR -5.94% [-7.11% to -4.86%]
clearItems2End phase estimated improvement -5ms [-9ms to -1ms] OR -8.38% [-14.33% to -1.88%]
render5000Items1End phase estimated improvement -236ms [-269ms to -203ms] OR -5.61% [-6.41% to -4.83%]
clearManyItems1End phase no difference [-5ms to 0ms]
render5000Items2End phase estimated improvement -263ms [-296ms to -231ms] OR -6.92% [-7.78% to -6.08%]
clearManyItems2End phase no difference [-4ms to 0ms]
render1000Items3End phase estimated improvement -67ms [-83ms to -53ms] OR -8.76% [-10.81% to -6.92%]
append1000Items1End phase estimated improvement -61ms [-73ms to -49ms] OR -6.9% [-8.24% to -5.53%]
append1000Items2End phase estimated improvement -25ms [-42ms to -8ms] OR -2.78% [-4.68% to -0.84%]
updateEvery10thItem1End phase no difference [-7ms to 8ms]
updateEvery10thItem2End phase no difference [-3ms to 3ms]
selectFirstRow1End phase estimated regression +1ms [0ms to 3ms] OR +0.52% [0.07% to 2.14%]
selectSecondRow1End phase no difference [0ms to 3ms]
removeFirstRow1End phase no difference [-1ms to 1ms]
removeSecondRow1End phase no difference [-1ms to 0ms]
swapRows1End phase no difference [0ms to 1ms]
swapRows2End phase no difference [0ms to 0ms]
clearItems4End phase no difference [-1ms to 6ms]
paint phase no difference [0ms to 0ms]

[16:19:59] Generating Benchmark Reports [started]
[16:20:04] Generating Benchmark Reports [completed]

Benchmark Reports    

JSON: /home/runner/work/glimmer-vm/glimmer-vm/tracerbench-results/compare.json

PDF: /home/runner/work/glimmer-vm/glimmer-vm/tracerbench-results/artifact-1.pdf

HTML: /home/runner/work/glimmer-vm/glimmer-vm/tracerbench-results/artifact-1.html

[NullVoxPopuli]

Some preliminary improvement here (development mode):

(at the cost of type safety)

[NullVoxPopuli]

In production mode, it looks like we get back to 5.6 performance
https://glimmer-vm-testing-3.ember-performance-testing-prod.pages.dev/?benchmarks=%5B%22Render%20complex%20html%20(%40glimmer%2Fcomponent)%22%2C%22Render%20complex%20html%20(template-only)%22%5D&emberVersions=%5B%225.5%22%2C%225.6%22%2C%225.7%22%2C%225.8%22%2C%225.9%22%2C%225.10%22%2C%22ember-canary%22%2C%22ember-canary-custom%22%5D

[chancancode]

(at the cost of type safety)

It seems like these were intended to be stripped/strippable in prod/turned into no-op, any idea why that isn’t happening? (or maybe the stripping was just never implemented?)

[NullVoxPopuli]

It seems like these were intended to be stripped/strippable in prod/turned into no-op, any idea why that isn’t happening? (or maybe the stripping was just never implemented?)

I think the idea was that it would be dependent on the user's terser config -- and that is not reliable.

There is a prod build in the glimmer packages, but it seems ember projects don't currently use it (this is something we could try to fix, but I think it'd be better to try to solve with a merged monorepo (glimmer-vm in to ember-source)), so we had less layers of build configs.
I also haven't been able to tell if the checks are removed in glimmer's prod build, as everything is minified / obfuscated.

At the same time, this change alone is not enough to regain lost perf, so there is more exploration to be had.

[chancancode]

It seems like these were intended to be stripped/strippable in prod/turned into no-op, any idea why that isn’t happening? (or maybe the stripping was just never implemented?)

I think the idea was that it would be dependent on the user's terser config -- and that is not reliable.

I'm not sure that is a real issue for a few reasons.

For one thing, Ember itself relies on the same kinds of transformations for its own assertions, etc, and if all of that is generally not working correctly, then we have bigger problems to worry about.

Also, if the transformation is active and working properly, it should ultimately result in something along the lines of:

import { someDebugStuff } from "@glimmer/debug/hopefully-stripped-in-prod";

// ...

if (false) {
  someDebugStuff(maybeExpensiveArgument());
}

That is the part that we would hope terser to remove – delete the branch altogether, then realize the import is unused and remove it (or maybe our transformations have already inlined the debug code and removed the import).

Even if that is not working properly due to terser config, the worst that could happen is bloating the bundle size a bit, which is not ideal but shouldn't have the kind of effect we are seeing. At runtime, the engines won't ever run the actual expensive checking code, evaluate its arguments, etc. So if that's all there is to it, I would expect the worse that could happen is a slightly inflated boot time but gets amortized away in the repeated runtime benchmarks.

So I would think that to whatever extent that debug code gets into the final Ember build at all, and, worse running at runtime, is indicative of a build configuration issue on our (or Ember's end), and #1599 seems to support that. That appears to account for most of the regression and lines up with the timeline (build refactors here, build refactors on Ember side, etc)

[NullVoxPopuli]

#1602

it's perhaps not just terser, or we need more inlining or something.

Or maybe it's possible that the way the checks are written -- they just aren't strippable.

They don't have any production swaps like assert does so I don't know how terser would remove them

[chancancode]

Yea it seems pretty clear to me the intent is to replace those check(expr, xyz) into just expr (then hopefully something else will realize xyz is not otherwise used and drop the import), but something we own has to be doing that rewrite.

Either there is a misconfiguration and that rewrite stopped happening, or @wycats wrote those checks intending to later write the transform but never did. If it’s the latter, assuming we are already doing these sort of rewrites for some other cases, it shouldn’t be too bad to add this case too.

The checks are doing a useful thing (both for typescript during development, and to catch otherwise difficult to catch bugs in dev mode/when running tests), just have to be dropped somehow in production

[NullVoxPopuli]

Closing, because #1606 solved a lot