Store an objects meta in a native WeakMap if present.

[rwjblue]

Primary benefits:

• Avoid polluting the object with extra properties (enumerable or not, it is still annoying).
• Avoid using Object.defineProperty for every object's meta creation.
• Unlocks future usage of meta for more things (function meta, hooks meta, view meta, etc) without adding even more obj[META_FIELD] property lookups.

@krisselden benchmarked this against a real app (in Chrome) and the performance was slightly better than the fallback path.

[rwjblue]

Tested crates.io against before and after this PR using chrome-tracing.

Average duration before: 35080.13
Average duration after: 33727.33

Gist with more details

[krisselden]

Generally we want boxplots which show 25th, 50th, 75th, and 1.5 IQR. I've tested roughly the same thing (a patched older ember with roughly the same change I gisted you and sent you the boxplots of) so I believe this is good to go, I'm just saying what I'd like to show

[rwjblue]

I did more thorough testing (thanks to @krisselden's R script). The app I ran against was still fairly small (the specific page profiled was https://crates.io/crates/solicit), but it shows that the change has a very small positive impact overall. The gist linked above, has been updated.

@krisselden - Since the upside to this change is fairly large (it unlocks many other things down the line), I'd like to go merge. Thoughts?

[rwjblue]

Chatted with @krisselden about this yesterday, and we are doing a bit more research into this.

• I need to eliminate the outliers in the benchmarks above and increase the iteration counts to ensure things are more consistent (I believe this is an issue with uncaptured network requests in the test app).
• There is some question about an older GC issue in Chrome when using WeakMaps (we believe it has aged out and not an issue, but we need to confirm).

[krisselden]

@rwjblue Godfrey and I tested this on Skylight as well and saw a slight improvement. @stefanpenner said we need to add a dev mode assert when WeakMap is supported assert that the object isExtensible so that we know the fallback path will work on a platform that doesn't support WeakMap.

[stefanpenner]

these numbers look promising, especially since we are actually getting a ergonomic win (no longer mutating all objects we observe etc).

The sample size does like quite small, I'm assuming you guys confirmed other browsers are pwnd by this?

[chancancode]

Actually, @krisselden was referring to a different/unrelated experiment. I ran a comparison between this branch (rebased) vs master, with or without the glimmer feature flag on a version of Skylight. I ran this for 50 iterations on Chrome 52.

The result (all times are in ms):

1. Without glimmer: https://cdn.rawgit.com/chancancode/90d4be1c803eb3b87a20695f17d402e0/raw/29cd90d2398efef1ec9cee092604fbdbea3fb320/1-htmlbars.html
2. With glimmer:
   https://cdn.rawgit.com/chancancode/90d4be1c803eb3b87a20695f17d402e0/raw/29cd90d2398efef1ec9cee092604fbdbea3fb320/2-glimmer.html

As you can see, the WeakMap version is slightly slower, but the difference is pretty small; as long as we are getting other wins out of it, I think it's fine.

I attached the logs here if anyone would like to check my work.

[stefanpenner]

@chancancode ya, even if its slightly worse, its just much much better. And honestly it is only slightly worse because we have miles of mega hacks to make the existing solution better. If we unwind those further i suspect we will be in a better place regardless.

I would also love (doesn't have to be thorough, but quick tests ensuring that no obvious perf regressions occur in our other browsers). In past tests they where fine, if they are still fine (handy wavy is ok) we should move forward with this. I just want to avoid unexpected surprises

[krisselden]

I have a thought about avoiding the walk on peekMeta of the proto chain I want to try that I think will make this perform much faster, I believe this also to be the path that will allow us to avoid mixing read/write during construction

[snuggs]

A little late to this party but can Symbols be used? I remember previously using the Object.getOwnPropertySymbols(object) API in this case. I know it was perfect for meta properties with zero chance of conflict (for the most part).
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertySymbols

by using this API one does not need to enumerate over string based property lookup. Not sure of the performance or if this is still considered "iterable-like" extra properties since they won't return when (traditionally/conventionally) iterated over.
http://www.2ality.com/2014/12/es6-symbols.html#symbols_as_keys_of_meta-level_properties

/cc @stefanpenner @krisselden @rwjblue

[stefanpenner]

@snuggs symbols are an interesting approach, the downside is that today they change the internal shape of the project they are attached to (which can lead to perf issues), and to a much lesser degree things like Proxies can intercept them and other mechanisms can be used to extract them.

WeakMap although strange at first, is more or less a synonym for private state. Although at first we may not enable this, in the future we can benefit from WeakMaps working on frozen objects etc.

Although symbols could fit the bill, I believe WeakMap is a better fit.

[rwjblue]

I just updated this PR, avoiding the prototype chain walking in peekMeta (as suggested by Kris above). There were 8 tests that failed as a result of this, all are doing things that seem somewhat dubious to me to begin with. We need to review these skipped tests and decide if they should be deleted or changed in some way to keep their original intent.

@chancancode - Can you run this again through ember-bench to check for performance regressions against current master?

[rwjblue]

Need to remove this before landing, I've been using these counters to help try different variations...

(this is also causing the node tests to fail due to the window usage)

[runspired]

If we can get a heimdall release and use heimdall counters, I have a babel transform to strip it out for builds so that we can leave this sort of instrumentation in permanently.

[rwjblue]

In case y'all are curious, here are the counts of this PR before and after the most recent update:

Original PR (walking prototype chain to find meta):

deleteCalls : 15636 
metaCalls : 403432 
metaInstantiated : 73223 
peekCalls : 744003 
peekPrototypeWalks : 137583 
setCalls : 73223

Updated PR:

deleteCalls: 15636
metaCalls: 403431
metaInstantiated: 73222
peekCalls: 817159
peekPrototypeWalks: 0
setCalls: 73222

[krisselden]

Just commenting on the failing tests, they aren't properly replicating the setup the Object model does, I'll review them see if they can be made to better reflect what currently happens.

It is my general feeling though, that if you apply a mixin to an object, then do Object.create() on it, this is not a supported thing, there are currently things that already break doing that, observers have cases they would fire across instances.

My feeling is manually extending an class should be done from Ember.Object, and you should be required to call the super ctor from your extension and it can setup the meta across the prototype chain.

[stefanpenner]

I believe core-js's would actually match this. As I believe it patches toString to doit...

[rwjblue]

@stefanpenner - It patches the Object.prototype.toString? That seems somewhat aggressive...

[rwjblue]

Yikes: https://github.com/zloirock/core-js/blob/master/modules/es6.object.to-string.js#L7-L9

[rwjblue]

Looks like lodash has been through this one already:

• https://github.com/lodash/lodash/blob/4.15.0/dist/lodash.js#L11583-L11589
• https://github.com/lodash/lodash/blob/4.15.0/dist/lodash.js#L3277-L3291
• https://github.com/lodash/lodash/blob/4.15.0/dist/lodash.js#L6089-L6098
• https://github.com/lodash/lodash/blob/4.15.0/dist/lodash.js#L1427-L1431

Quoting (just before _.isNative throws when it detects core-js):

 * **Note:** This method can't reliably detect native functions in the presence
 * of the core-js package because core-js circumvents this kind of detection.
 * Despite multiple requests, the core-js maintainer has made it clear: any
 * attempt to fix the detection will be obstructed. As a result, we're left
 * with little choice but to throw an error. Unfortunately, this also affects
 * packages, like [babel-polyfill](https://www.npmjs.com/package/babel-polyfill),
 * which rely on core-js.

[stefanpenner]

c, that is most likely a good way to detect.

[rwjblue]

After a number of performance tests I'd like to land this into the 2.9 beta cycle. There is a very slight regression (roughly 0.003%), but I believe that the resulting improvements will have a higher positive impact on performance (things like moving GUID_KEY to the same system).

The latest performance tests are here.

There are ~ 8 tests that have been skipped that I believe we need to rewrite or remove, but we can iterate on that after landing, unless there are objections...

@emberjs/commiters - Thoughts?

[chancancode]

small correction: the difference is 0.3% 😄

[stefanpenner]

@rwjblue do you want me to review the skipped tests?

[rwjblue]

small correction: the difference is 0.3% 😄

Derp. Thank you.

[rwjblue]

do you want me to review the skipped tests?

@stefanpenner - Ya, absolutely if you have time.

[stefanpenner]

There are ~ 8 tests that have been skipped that I believe we need to rewrite or remove, but we can iterate on that after landing, unless there are objections...

@rwjblue the skipped tests are actually one of the reasons why I didn't land this already. But I thought the breakthrough was that @krisselden demonstrated that the prototype walk to implement this was sufficiently fast?

Ultimately (we should get consensus from @wycats / @tomdale / @krisselden), but if it is mega perf issue, I would gladly trade pure O.create for WeakMap associated meta working. That being said, it is quite possible someone relies on this functionality today.

Its worth pointing out that these tests can't be rewritten, with the exception of creating a new create method ala. Ember.createWithMeta instead of pure Object.create(x). Because their "essence" is explicitly that meta works correctly with pure prototypal inheritance alaObject.create.

[rwjblue]

@stefanpenner - Yes, we had a prototype walking setup before the last update (and these tests did pass with it), but we removed that because we suspected it to be a fairly decent performance win (and that it would make our performance better than the non-WeakMap version). As you can see from the two different perf test runs (linked below) it didn't end up changing the performance nearly at all. I think we can put the prototype walk back (then these tests will pass).

• Results with prototype walk
• Results without prototype walk

[stefanpenner]

@rwjblue is the perf test hitting that scenario much? I'll gladly review the perf test (if i can be pointed to a link) to help confirm we are indeed hitting the "in question" scenario. If indeed the perf hit is negligible, I would prefer to keep it in.

[rwjblue]

@stefanpenner - The perf test is running against a "medium" dataset in a real app (Skylight), we'd have to chat with @chancancode to determine if it is exercising the prototype walking code. Alternatively, I can add it back here with its own counter, and we can have him let us know what the resulting counters for the prototype walk are (in a debug build).

[krisselden]

@stefanpenner I have many doubts about some of these tests. They don't actually setup inheritance correctly. I'm OK with a walk on apply that sets stuff up but on peek seems dubious to me, I think inheritance requires ceremony to say the meta is now describing a prototype.

There is a reason these tests can be skipped and you can boot your app still.

[krisselden]

this idea meta could correct itself already is only half baked, it is not going to cache and observe correctly if you had started using it as an instance and suddenly switched to the role of being a shared object

[krisselden]

@rwjblue we should try ember bench on ember observer

[krisselden]

If someone were to rely on this I feel sorry for them troubleshooting the bugs they'll encounter, these tests are simplistic at best in their coverage of what can go wrong with shared state in the prototype meta in this scenario.

[krisselden]

@stefanpenner but you are correct that it did not seem to make a difference to remove it, I just have concerns these tests are writing a check we can't cash

[stefanpenner]

@rwjblue / @krisselden / @chancancode for reference, a scenario that should hit the "Walking prototype path", would be observing pojo's, or foreign objects. Luckily, pojos typically have a really short prototype chain, which may be why the perf difference is negligible.

[rwjblue]

The performance results from the last run (which did not include the prototype walking to find the current meta object) showed that the performance is essentially the same as what it was with the prototype walk and passes all the existing tests.

I have updated this PR to use the prototype walking system (which should pass the tests that were previously skipped).

Assuming that CI is happy with this now, I would like to land in canary so we can continue to iterate forward with the next phases of optimizations that this unlocks.

[stefanpenner]

@rwjblue should we also move GUID_KEY over eventually?

[rwjblue]

Yep, I am working on a PR for that literally at the moment 😉