get() / set() perf optimizations#17166
Conversation
| let map = pointer._descriptors; | ||
| if (map !== undefined) { | ||
| for (let key in map) { | ||
| map.forEach((value, key) => { |
There was a problem hiding this comment.
Note: I use Map.forEach() for the iteration, since it is the only method that works with IE11.
|
These changes all seem pretty good to me, thanks for working on this! |
|
@krisselden - Mind taking a look? |
|
It might also be possible to remove the |
|
Looks like some linting failures, mind taking a look? |
| let hasMeta = meta !== undefined; | ||
|
|
||
| if (hasMeta && (meta.isInitializing() || meta.isPrototypeMeta(obj))) { | ||
| if (meta !== null && (meta.isInitializing() || meta.isPrototypeMeta(obj))) { |
There was a problem hiding this comment.
why not check once and reference it like before let hasMeta = meta !== null ?
There was a problem hiding this comment.
Because the ts linter won't recognize that meta is non-null, and the subsequent statements would need a "!":
if (hasMeta && (meta!.isInitializing() || meta!.isPrototypeMeta(obj))) {| export function _getPath<T extends object>(root: T, path: string | string[]): any { | ||
| let obj: any = root; | ||
| let parts = path.split('.'); | ||
| let parts = typeof path === 'string' ? path.split('.') : path; |
There was a problem hiding this comment.
I think it will be even more performant if _getPath only accept array of path, https://github.com/emberjs/ember.js/pull/17166/files#diff-45589baa0b0954cefba717cf63c270f9R100 and here you can put isObjectLike ? _getPath(obj, keyName.split('.')) : undefined
There was a problem hiding this comment.
Yes, I thought about that, and you might be right. I discarded it as a micro optimization I didn't want to do in this pass. It is not likely to make any measurable difference.
|
I had similar approach in https://github.com/emberjs/ember.js/pull/17057/files but gave up since it was linting nightmare :) |
|
this makes #17058 redundant since it is addressed here already |
|
@bekzod I actually found the typescript very helpful doing the patch. I doubt I would have dared to do it without. |
|
of course typescript is very helpful for such refactors, but when I did #17057 it required a lot of |
|
@rwjblue I fixed the linting issue. |
|
@kanongil we chatted about this a bit in discord a week or two ago, you were working on getting ember-macro-benchmark running with this change to ensure there is not a regression outside of microbenchmarks. Have you had a chance to do that yet? |
|
Yes, sorry about the delay. It took quite a bit of effort to get ember-macro-benchmark to work properly. Initially, it wouldn't patch the ember source into the project, but I think I got it solved. I managed to get it to work with emberaddons.com and this config: {
"har": "archives/www.emberaddons.com.har",
"runCount": 100,
"cpuThrottleRate": 4,
"servers": [{
"name": "master-e2352e6",
"port": 8880,
"ember": "embers/ember-master-e2352e6.min.js"
}, {
"name": "get-set-perf-d6e5472",
"port": 8881,
"ember": "embers/ember-get-set-perf-d6e5472.min.js"
}]
}Result: results.pdf |
|
OK, great, so the results definitely don't show a regression (suggests that the change is statistically insignificant for emberaddons.com). IMHO, this code is strictly better than what we had before which means that as long as we do our due diligence to confirm it does not introduce a regression (which it seems that we have) we should land... |
|
@krisselden - Do you to do one more pass at review before landing? |
|
Looks like a recent PR merged made this have conflicts, mind fixing that up? |
This avoids redundant peeking when passing empty meta to certain methods
|
Just rebased to fix conflicts, will merge once CI is done... |
|
@kanongil - Thank you for pushing this forward! |
I noticed that the ember-performance Chrome scores of
get()andset()dropped dramatically whenember-metal-es5-getterswas enabled, so I had a look at it.I have managed to mostly bring it back for
get(), and improve it forset()using the included patches. Both are roughly 2x the canary performance. Additionally, it seems to have improved therun()benchmark as well.All the patches should work individually, in case you only think some of them are suitable.
Chrome
Safari