Skip to content
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

get() / set() perf optimizations #17166

Merged
merged 5 commits into from Jan 2, 2019

Conversation

Projects
None yet
3 participants
@kanongil
Copy link
Contributor

kanongil commented Oct 31, 2018

I noticed that the ember-performance Chrome scores of get() and set() dropped dramatically when ember-metal-es5-getters was enabled, so I had a look at it.

I have managed to mostly bring it back for get(), and improve it for set() using the included patches. Both are roughly 2x the canary performance. Additionally, it seems to have improved the run() benchmark as well.

All the patches should work individually, in case you only think some of them are suitable.

Chrome

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.20 Safari/537.36

.------------------------------------------------------------------------.
|                   Ember Performance Suite - Results                    |
|------------------------------------------------------------------------|
|         Name         |       Speed        | Error  | Samples |  Mean   |
|----------------------|--------------------|--------|---------|---------|
|  -- Ember 3.0.0 --   |                    |        |         |         |
| Ember.get            | 1,626,872.70 / sec | ∓1.08% | 64      | 0.00 ms |
| Ember.run            | 670,554.44 / sec   | ∓2.42% | 65      | 0.00 ms |
| Ember.set            | 606,311.51 / sec   | ∓1.12% | 64      | 0.00 ms |
|  -- Ember 3.4.5 --   |                    |        |         |         |
| Ember.get            | 770,484.17 / sec   | ∓1.65% | 61      | 0.00 ms |
| Ember.run            | 710,395.41 / sec   | ∓6.42% | 58      | 0.00 ms |
| Ember.set            | 408,123.26 / sec   | ∓0.47% | 65      | 0.00 ms |
|  -- Ember canary --  |                    |        |         |         |
| Ember.get            | 858,315.30 / sec   | ∓0.96% | 61      | 0.00 ms |
| Ember.run            | 739,482.59 / sec   | ∓0.34% | 66      | 0.00 ms |
| Ember.set            | 453,404.65 / sec   | ∓0.61% | 65      | 0.00 ms |
|  -- Ember dev --     |                    |        |         |         |
| Ember.get            | 1,487,276.86 / sec | ∓1.04% | 63      | 0.00 ms |
| Ember.run            | 818,811.25 / sec   | ∓0.53% | 66      | 0.00 ms |
| Ember.set            | 996,207.06 / sec   | ∓1.33% | 61      | 0.00 ms |
'------------------------------------------------------------------------'

Safari

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0.1 Safari/605.1.15

.------------------------------------------------------------------------.
|                   Ember Performance Suite - Results                    |
|------------------------------------------------------------------------|
|         Name         |       Speed        | Error  | Samples |  Mean   |
|----------------------|--------------------|--------|---------|---------|
|  -- Ember 3.0.0 --   |                    |        |         |         |
| Ember.get            | 1,024,541.17 / sec | ∓0.93% | 62      | 0.00 ms |
| Ember.run            | 216,263.15 / sec   | ∓3.47% | 37      | 0.00 ms |
| Ember.set            | 711,296.65 / sec   | ∓0.99% | 64      | 0.00 ms |
|  -- Ember 3.4.5 --   |                    |        |         |         |
| Ember.get            | 660,504.61 / sec   | ∓0.83% | 62      | 0.00 ms |
| Ember.run            | 227,034.91 / sec   | ∓3.44% | 36      | 0.00 ms |
| Ember.set            | 393,704.49 / sec   | ∓0.93% | 63      | 0.00 ms |
|  -- Ember canary --  |                    |        |         |         |
| Ember.get            | 888,268.20 / sec   | ∓0.96% | 61      | 0.00 ms |
| Ember.run            | 236,605.91 / sec   | ∓3.75% | 36      | 0.00 ms |
| Ember.set            | 490,658.82 / sec   | ∓0.80% | 63      | 0.00 ms |
|  -- Ember dev --     |                    |        |         |         |
| Ember.get            | 952,592.90 / sec   | ∓0.96% | 63      | 0.00 ms |
| Ember.run            | 256,341.28 / sec   | ∓3.60% | 37      | 0.00 ms |
| Ember.set            | 690,086.43 / sec   | ∓1.09% | 62      | 0.00 ms |
'------------------------------------------------------------------------'
@@ -480,16 +494,15 @@ export class Meta {
while (pointer !== null) {
let map = pointer._descriptors;
if (map !== undefined) {
for (let key in map) {
map.forEach((value, key) => {

This comment has been minimized.

Copy link
@kanongil

kanongil Oct 31, 2018

Author Contributor

Note: I use Map.forEach() for the iteration, since it is the only method that works with IE11.

@rwjblue rwjblue requested a review from krisselden Oct 31, 2018

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Oct 31, 2018

These changes all seem pretty good to me, thanks for working on this!

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Oct 31, 2018

@krisselden - Mind taking a look?

@kanongil

This comment has been minimized.

Copy link
Contributor Author

kanongil commented Oct 31, 2018

It might also be possible to remove the descriptorFor() call in get(), since the value can simply be obtained by accessing the attribute (calling any getter). I skipped it for now – mostly because I'm not sure if there are any implications I'm missing from doing this.

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Nov 1, 2018

Looks like some linting failures, mind taking a look?


if (hasMeta && (meta.isInitializing() || meta.isPrototypeMeta(obj))) {
if (meta !== null && (meta.isInitializing() || meta.isPrototypeMeta(obj))) {

This comment has been minimized.

Copy link
@bekzod

bekzod Nov 1, 2018

Contributor

why not check once and reference it like before let hasMeta = meta !== null ?

This comment has been minimized.

Copy link
@kanongil

kanongil Nov 1, 2018

Author Contributor

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))) {
let obj: any = root;
let parts = path.split('.');
let parts = typeof path === 'string' ? path.split('.') : path;

This comment has been minimized.

Copy link
@bekzod

bekzod Nov 1, 2018

Contributor

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

This comment has been minimized.

Copy link
@kanongil

kanongil Nov 1, 2018

Author Contributor

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.

@bekzod

This comment has been minimized.

Copy link
Contributor

bekzod commented Nov 1, 2018

I had similar approach in https://github.com/emberjs/ember.js/pull/17057/files but gave up since it was linting nightmare :)

@bekzod

This comment has been minimized.

Copy link
Contributor

bekzod commented Nov 1, 2018

this makes #17058 redundant since it is addressed here already

@kanongil

This comment has been minimized.

Copy link
Contributor Author

kanongil commented Nov 1, 2018

@bekzod I actually found the typescript very helpful doing the patch. I doubt I would have dared to do it without.

@bekzod

This comment has been minimized.

Copy link
Contributor

bekzod commented Nov 1, 2018

of course typescript is very helpful for such refactors, but when I did #17057 it required a lot of ! signs had similar issues #17166 (comment)

@kanongil

This comment has been minimized.

Copy link
Contributor Author

kanongil commented Nov 11, 2018

@rwjblue I fixed the linting issue.

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Dec 7, 2018

@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?

@kanongil

This comment has been minimized.

Copy link
Contributor Author

kanongil commented Dec 7, 2018

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

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Dec 7, 2018

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...

@rwjblue

rwjblue approved these changes Dec 7, 2018

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Dec 7, 2018

@krisselden - Do you to do one more pass at review before landing?

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Dec 7, 2018

Looks like a recent PR merged made this have conflicts, mind fixing that up?

@rwjblue rwjblue force-pushed the kanongil:get-set-perf branch from d6e5472 to 89acaf9 Jan 2, 2019

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Jan 2, 2019

Just rebased to fix conflicts, will merge once CI is done...

@rwjblue rwjblue merged commit c2623b9 into emberjs:master Jan 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Jan 2, 2019

@kanongil - Thank you for pushing this forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.