[BUGFIX beta] ES6 classes on/removeListener and observes/removeObserver interop v2

[pzuraq]

This is a rework of #16874 which flattens and caches the state of event listeners more efficiently. Rather than rebuild the result of a matchListeners query each time, including deduping, we flatten the listeners down the hierarchy of metas the first time an event match is requested. This still defers the majority of the work early on (adding listeners is cheap) but also prevents us from having to do the work again later.

Should probably run ember-macro-benchmark against this to see if there is any notable perf difference before we merge. I'd also like to see if we can scope the revision counter down to a single hierarchy, so adding an observer on a prototype doesn't affect every class in the app.

cc @krisselden

[rwjblue]

Slight clarification, since the global revision is actually global this doesn’t seem to be about “somewhere up the inheritance chain”, right?

[pzuraq]

True, should probably read "somewhere up a class's inheritance chain"

[pzuraq]

Benchmark against master, looks like a slight improvement 😄

Correction, my setup was wrong and I was testing master against master 🙃Ran the benchmarks again (made sure they were serving the correct thing this time) and got slightly worse results. Unsure if they're statistically relevant either, will wait on @krisselden to review:

results.pdf
results.json.txt

[pzuraq]

Ran benchmarks against emberobserver.com as well and ended up with no significant results (double checked to confirm that substituting was happening correctly). It seems like emberaddons.com is doing something differently that causes the issue, going to add some counters to see if we can find any likely suspects.

results.pdf
results.json.txt

[pzuraq]

Quick update on research the past week into the regression. So far we've done a few different tests, including:

1. Running benchmarks against both emberaddons.com and emberobserver.com
2. Adding counters to various actions related to listeners, including adding, removing, and flattening/reopening listeners
3. Recording arguments passed to addToListeners and removeFromListeners to see what types of listeners are being added and removed, as well as recording what types of listeners are inherited
4. Attempting to rewrite the flattening logic to use different forms of array operations (concat, shift, unshift, slice)
5. Attempting to ensure references are not shared by duplicating inherited listeners rather than inheriting directly by reference.

We weren't able to affect the regression in any significant way by rewriting the code as is (tests 4 and 5). However, we gathered a few useful datapoints from the other tests:

• The regression occurs only on emberaddons.com, emberobserver.com is not affected.
• No reopen/reflattens were triggered on either website.
• emberaddons.com has a significant number of listeners that are added via inheritance, whereas emberobserver.com does not. This appeared to be the only significant difference in counters between the two.
• No FunctionListeners were inherited in emberaddons.com. All inherited listeners were a single string based listener, an observer on the EmberData Model class.

In discussing with @krisselden this morning we revisited the fact that the current in-master version of the code does not always finalize listeners, and will instead walk the inheritance chain of metas upwards until the root (or until the first finalized meta is found). It may be that building the caches is more expensive than walking the inheritance chain in most cases.

Today I attempted to test this hypothesis by isolating the individual changes in this PR and running the benchmarks again. Here are the results and links to the individual commits

1. Converting listeners to objects (No regression)

   • commit
   • results.json
   • results.pdf

2. Always finalizing listeners before matching (Regression)

   • commit
   • results.json
   • results.pdf

3. Replacing the finalizing logic with flattening logic, but only flattening in the case of a removal (No regression)

   • commit
   • results.json.txt
   • results.pdf

4. Always flattening listeners (current PR) (Regression)

[pzuraq]

Further testing this PR on a large application showed no noticeable regression, which means that for two applications (including emberobserver.com) we saw no statistically significant change in performance.

[chadhietala]

This likely should be

if (method === undefined) {
  //...
} else {
  //...
}

[btecu]

Incomplete statement

[pzuraq]

After a bit more work we managed to reduce the perf regression to a statistically insignificant amount. The changes were:

1. We used the parent's listeners when we the child doesn't have unique listeners, AND we cache them locally so we don't have to recurse upwards every time (the caching was pretty important it turned out).
2. Inlined the setFlattened/isFlattened/shouldFlatten methods. It actually made a minor but measurable impact, which is weird because they should have been inlined by the optimizing compiler.
3. We were creating an array for every call to a method, and making that lazy added a tiny boost.

We agreed that this PR should be merged for now, and that we should continue to investigate the perf characteristics as we have time since we did think this was originally going to be an improvement, and there are some odd behaviors here.

The updates to the deprecation guide are also ready for review.

results.pdf

[rwjblue]

Updated PR to indicate [BUGFIX beta] since we want this in 3.6...