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

Drop EmptyObject and use Object.create(null) again #15001

Closed
bmeurer opened this issue Mar 11, 2017 · 11 comments

Comments

@bmeurer
Copy link
Contributor

commented Mar 11, 2017

It seems that new EmptyObject was added as a work-around for Object.create(null) performance. This has a couple of drawbacks for Ember itself, but also comes with a performance penalty in recent Chrome versions (i.e. in all current channels) that include this CL.

From what I can tell, new EmptyObject is often used to create objects that are supposed to be used as dictionaries. But at least in V8 function constructors always create fast mode objects, which means Ember probably wastes (metadata) memory in the browser plus time just to let the engine figure out that it should essentially transition these objects to dictionary mode objects. Object.create(null) always creates a dictionary mode object from the start (since the aforementioned CL landed), and it's compatible with jQuery.isPlainObject and other stuff. The allocation is roughly on par with the performance of new EmptyObject in all current Chrome channels.

So I'd like to trigger a discussion to get rid of EmptyObject.

/cc @krisselden @stefanpenner

@rwjblue

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

We have discussed previously and I believe we are 👍 .

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

@bmeurer sounds great. This was totally just added as work around. I am slightly nervous about performance on older androids but.... may be time?

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

working on a PR for testing PR for testing -> #15002


@bmeurer in the past, we also found forcing some of our objects into dictionaries sooner yielded some improvements. Is this something we should continue? https://github.com/emberjs/ember.js/blob/master/packages/ember-utils/lib/dictionary.js#L8-L18 Or should we discontinue.


@bmeurer We also worked around the cost of Object.create in other ways, might you have some time to discuss those scenarios in more detail over the next week? Specifically code in Meta like _getInherited was basically created to work around some issue (the results where good, but what remains ends up being complicated and costly on its own).

@bmeurer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2017

@stefanpenner The delete trick is a common work-around, i.e. older TypeScript versions used that as well. We can chat about it next week, maybe do a VC in the morning (european time)?

As for older Android: Chrome is ever-green, so it'll update. For the other Chromium-based Android browsers, there's not a lot we can do unfortunately, and many of them are still at V8 4.x, where the new Ember code will probably perform poorly anyway. Not sure if you do performance tracking for those old versions.

stefanpenner added a commit that referenced this issue Mar 12, 2017
stefanpenner added a commit that referenced this issue Mar 12, 2017
@stefanpenner

This comment has been minimized.

Copy link
Member

commented Mar 12, 2017

We can chat about it next week, maybe do a VC in the morning (european time)?

Work for me, any day except for Wednesday (my evening, thursday your morning) works for me. I'll ping you on hangouts

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Mar 12, 2017

added patch for ember-data: emberjs/data#4854

@homu homu closed this in c06d82d Mar 14, 2017
homu added a commit that referenced this issue Mar 14, 2017
[Fixes #15001] Remove internal EmptyObject usage

- [x] confirm all are correctly removed
- [x] confirm tests pass
- [ ] do some tests

----

If this ends up good, we should likely consider deprecating `Ember.EmptyObject`.

---

cc @bmeurer
wycats pushed a commit that referenced this issue Mar 14, 2017
@verwaest-zz

This comment has been minimized.

Copy link

commented Mar 14, 2017

Note that as we speak we're also adding support for {__proto__:null} which will probably be even faster (can be fully recognized in the parser). Object.create(null) will of course be available slightly sooner.

@TimothyGu TimothyGu referenced this issue Mar 19, 2017
2 of 2 tasks complete
TimothyGu added a commit to TimothyGu/node that referenced this issue Mar 23, 2017
After V8 5.6, using Object.create(null) directly is now faster than
using a constructor.

Refs: emberjs/ember.js#15001
Refs: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
TimothyGu added a commit to TimothyGu/node that referenced this issue Mar 24, 2017
After V8 5.6, using Object.create(null) directly is now faster than
using a constructor for map-like objects.

PR-URL: nodejs#11930
Refs: emberjs/ember.js#15001
Refs: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca

This comment has been minimized.

Copy link

commented Mar 27, 2017

@bmeurer allocation seems to be 5/6x slower on current Node.js (v7.7.4 - V8 5.5) and Node.js master (V8 5.7).

$ node index.js 
new EmptyObject() x 59,497,668 ops/sec ±2.33% (80 runs sampled)
Object.create(null) x 9,925,380 ops/sec ±1.52% (83 runs sampled)
Fastest is new EmptyObject()

$ ./node-master@88daf88 index.js 
new EmptyObject() x 49,234,553 ops/sec ±2.63% (82 runs sampled)
Object.create(null) x 8,865,356 ops/sec ±1.19% (84 runs sampled)
Fastest is new EmptyObject()

It's the same when enabling the TurboFan compiler.

$ ./node-master@88daf88 --turbo index.js 
new EmptyObject() x 21,109,258 ops/sec ±2.32% (72 runs sampled)
Object.create(null) x 21,887,499 ops/sec ±1.57% (77 runs sampled)
Fastest is Object.create(null)
@bmeurer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

@lpinca You probably didn't measure it's use as a dictionary. If you also include that, then Object.create(null) is a lot faster.

@lpinca

This comment has been minimized.

Copy link

commented Mar 27, 2017

@bmeurer I was only measuring how much it takes to create the object as we are thinking about reverting the change in Node.js (see nodejs/node#11930 (comment)) for this reason which is unfortunate.

@bmeurer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

@lpinca Yap, the allocation is only inlined into TurboFan, and as such escape analysis can only remove the allocation in TurboFan. But an Object.create(null) that doesn't escape isn't very useful.

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