-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX beta] ES6 classes on/removeListener and observes/removeObserver interop v2 #16923
Conversation
10a32f7
to
ca3b1eb
Compare
|
||
/** | ||
Flattening is based on a global revision counter. If the revision has | ||
bumped it means that somewhere up the inheritance chain something has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight clarification, since the global revision is actually global this doesn’t seem to be about “somewhere up the inheritance chain”, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, should probably read "somewhere up a class's inheritance chain"
ca3b1eb
to
87897d6
Compare
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: |
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. |
Quick update on research the past week into the regression. So far we've done a few different tests, including:
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:
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
|
6b685a0
to
7f28ab3
Compare
Further testing this PR on a large application showed no noticeable regression, which means that for two applications (including |
7f28ab3
to
3d24e60
Compare
metaFor(obj).removeFromListeners(eventName, target, method!); | ||
let m = metaFor(obj); | ||
|
||
if (!method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely should be
if (method === undefined) {
//...
} else {
//...
}
29e1c1c
to
f7e5b2b
Compare
…terop v2 This is a rework of emberjs#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.
f7e5b2b
to
ef21456
Compare
let listeners = this.flattenedListeners(); | ||
|
||
if (DEBUG) { | ||
counters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete statement
8ac3b38
to
4e24e8e
Compare
4e24e8e
to
506b884
Compare
After a bit more work we managed to reduce the perf regression to a statistically insignificant amount. The changes were:
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. |
Updated PR to indicate |
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