[FEATURE] Tracked Props Performance Tuning

[pzuraq]

This PR represents a journey I've taken over the past 2 weeks, tuning and tweaking Ember and Glimmer to try to make up the regression caused by enabling tracked properties. The TL;DR: With all of the work done in this PR and its companion over in Glimmer, the regression has been reduced by ~90% in emberobserver.com, from ~413ms to ~48ms (with 8x CPU slowdown).

Here are the results, and the data that backs them:

results.pdf
results.json.txt

There are a number of optimizations that were made to make this happen. Each individual test case represents one step in those, and I'll cover each in detail. The commits for these steps are as follows:

Test Case Name	Ember.js Commit	Glimmer-VM Commit
master	49ea6fe9e	5de7da10
ember-optimizations	905ee3cf4	5de7da10
glimmer-bugfixes	905ee3cf4	6f3680a9
make-updatable-tags-dirtyable	8f9991a2a	6f3680a9
better-caching	8f9991a2a	84afef73
monomorphic-validation	8f9991a2a	1c0a90ce
monomorphic-tag	d0d0339ba	599467ee
tracked-disabled	b6a3ecdb6	5de7da10

The Glimmer portion of the work is in this PR: glimmerjs/glimmer-vm#963

Big thanks to @krisselden for helping out with this work!

ember-optimizations

Ember Diff 1
Ember Diff 2

The first step in optimizing was to try to optimize the new code in Ember itself. The feature work had been a bit hasty, so there were definitely a few opportunities in there. The optimizations include:

• Avoiding accessing the last node in a chain when looking up tags. For example, when looking up foo.bar.baz for a CP dependency, we need the tags for foo, bar, and baz. We need to actually access foo to get bar's tag, and then we need to access bar to get baz's tag, but we don't need to access baz, since its the end of the chain.
• Simplifying the chain tag lookup to use substrings of the path, rather than splitting the string and creating arrays
• Simpliying computeds so they don't create unnecessary arrays
• Fixing a bug in aliases so they don't recreate their tag chains on every single access
• Adding an untrack feature that allows us to explicitly disable autotracking and avoid creating/tracking tags
• Using a map instead of a 2D array for lazy tag lookups
• Flattening tag chains to minimize the number of combinator tags that are created

Each of these micro-optimizations ultimately led to a ~125ms improvement in performance. So far, things were looking good, and we seemed to be on track!

glimmer-bugfixes

Glimmer Diff

Unfortunately, we also uncovered a few bugs in Glimmer that needed to be fixed. Specifically, Glimmer's tags couldn't handle cycles, and their caching was too aggressive.

The cycles issue was not overall too much of a hit to fix. The bigger regression was in fixing caching. They would cache immediately on being calculated, and later on if they were updated to point to a new tag, this update would not bust their cache. We do these updates during the computation of a reference's value, so by the time the reference was done calculating, the tag's cache (set before the reference started calculating) was already stale.

The fix was to bump the global revision counter for every update. This meant that the act of updating state based on "real" dirtiness would cause us to recheck many, many tags (counts jumped from lows 1000s of checks to hundreds of thousands). This set us back quite a bit, leading to a ~177ms regression, setting us back to a worse place than we started.

make-updatable-tag-dirtyable

Ember Diff

Up until this point, there was an UpdatableTag class and a DirtyableTag class, but not one that could do both, which is what Ember needed. We were using a combinator tag to combine the two for every property tag in Ember, which was a fair amount of work, and meant we had that many more layers of tags to check and recalculate frequently due to the caching fixes.

I originally began by backporting the UpdatableDirtyableTag that is in Glimmer's master branch, but didn't see as much on an improvement as I thought I would. This pointed to some kind of polymorphism being introduced, where a code path that was used to running for only one type of object regressed because it now had to deal with more types. Sure enough, combining the UpdatableDirtyableTag APIs into UpdatableTag proved to be a much better improvement.

With this change, we saw a ~161ms improvement, which almost fixed the regression caused by our bugfixes.

better-caching

Glimmer Diff

The caching was clearly a huge bottleneck in tag calculation, especially the re-dirtying that could be caused by updates. The thing was, updates also didn't really need to bump the global revision count. They didn't signal a change in state, as much as a change in organization of tags.

We tried a few different strategies here, including separating out the validation logic so it wouldn't eagerly calculate and cache the value, and that helped a bit, but not as much as we would have liked. After thinking about it some more, I realized a few things were true here:

• When we calculate a tag's value, it will necessarily be <= the current revision count
• When we update a tag to point to a new tag, the new tag's value will also necessarily be <= the current revision count
• Later, when we are checking to see if the value the tag represents is stale, stale tags will necessarily have a value > the revision count at the time when the value was last checked

Using these principles, we can make a large change here, and have all tags always return the current revision count when we ask for their value. We can then compare the tag's own "inner" value against the last revision count to know if anything has changed in the tag since the last revision bump.

This change brought the counts for cache busts/checks back down into the 1000s, and netted us a ~121ms improvement.

monomorphic-validation

Glimmer Diff

The fact that the UpdatableDirtyableTag fix showed more polymorphism was bugging me still, so I returned to that next. In looking at the code for validators, I realized that many tags shared methods via subclassing. These methods would interact with different shaped objects all the time, each of the subclassed tags.

Duplicating the logic they shared via subclassing directly into each subclass resulted in ~36ms improvement, not the best change but still a decent chunk, pointing to polymorphism.

monomorphic-tag

Glimmer Diff
Ember Diff

The final optimization was to replace all of the tag classes, except CURRENT_TAG which had to have its own class, with a single class. This also allowed us to get rid of the TagWrapper class (which existed to prevent polymorphic access) and simplify tag logic quite a bit.

This was a relatively large bump, showing a ~93ms improvement over the last change.

This is also definitely the largest change in this series. It would simplify tags, but also make them less flexible, and potentially easier to abuse since any tag can do any operation (combinations, updates, dirties). This may be less of an issue if we switch to a more functional API for tags, since it would allow us to control their usage more.

[rwjblue]

@pzuraq - Looks like there are some conflicts here, mind rebasing?

[pzuraq]

Yeah, I was aware of those, I didn't want to rebase until people have had a chance to review, since each build is based on a commit currently, and the commit list is up top. Given we'd have to rebase and rerun the tests (which took a while), and the Glimmer changes have to be merged and released first anyways, I figured it would be alright to review in isolation.

[lifeart]

Just in case if it may be helpful

[pzuraq]

Yeah, definitely aware of this! I always make sure to do explicit checks in hot paths

[rwjblue]

@pzuraq - How goes the work on this?

[pzuraq]

glimmerjs/glimmer-vm#963 is currently ready for review, pending on @wycats and @krisselden currently. Once they review and approve, we can merge that, cut a release, and rebase/merge this PR.

[rwjblue]

Awesome, thank you. Just a reminder for when this is rebased to prep for landing: we'll need to prefix the commits with [BUGFIX beta] to ensure they get pulled down into the beta branch for 3.13.

[rwjblue]

Looks like some TS failures from upgrading:

$ yarn lint:tsc && yarn lint:tslint && yarn lint:eslint && yarn lint:docs
$ tsc --noEmit
packages/@ember/-internals/glimmer/lib/modifiers/action.ts(7,10): error TS2305: Module '"../../../../../../node_modules/@glimmer/reference/dist/types"' has no exported member 'RevisionTag'.
packages/@ember/-internals/glimmer/lib/modifiers/action.ts(7,23): error TS2305: Module '"../../../../../../node_modules/@glimmer/reference/dist/types"' has no exported member 'TagWrapper'.
packages/@ember/-internals/glimmer/lib/utils/references.ts(30,3): error TS2724: Module '"../../../../../../node_modules/@glimmer/reference/dist/types"' has no exported member 'VALIDATE'. Did you mean 'validate'?
packages/@ember/-internals/glimmer/lib/utils/references.ts(32,3): error TS2724: Module '"../../../../../../node_modules/@glimmer/reference/dist/types"' has no exported member 'VALUE'. Did you mean 'value'?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn lint" exited with 1.

[pzuraq]

The assert issue was being caused by Edge 44.17763.1.0, really weird bug 😕