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

Store an objects meta in a native WeakMap if present. #13783

Merged
merged 1 commit into from Sep 25, 2016

Conversation

Projects
None yet
6 participants
@rwjblue
Member

rwjblue commented Jul 2, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jul 4, 2016

Member

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

Member

rwjblue commented Jul 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jul 4, 2016

Member

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

Member

krisselden commented Jul 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jul 5, 2016

Member

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.

boxplot

histogram

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

Member

rwjblue commented Jul 5, 2016

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.

boxplot

histogram

@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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jul 6, 2016

Member

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

rwjblue commented Jul 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Aug 12, 2016

Member

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

Member

krisselden commented Aug 12, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 12, 2016

Member

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?

Member

stefanpenner commented Aug 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Aug 17, 2016

Member

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.

Member

chancancode commented Aug 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 18, 2016

Member

@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

Member

stefanpenner commented Aug 18, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Aug 20, 2016

Member

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

Member

krisselden commented Aug 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@snuggs

snuggs Aug 20, 2016

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

snuggs commented Aug 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 22, 2016

Member

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

Member

stefanpenner commented Aug 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 10, 2016

Member

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?

Member

rwjblue commented Sep 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 10, 2016

Member

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
Member

rwjblue commented Sep 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 10, 2016

Member

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.

Member

krisselden commented Sep 10, 2016

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.

let instance = new WeakMap();
// use `Object`'s `.toString` directly to prevent us from detecting
// polyfills as native weakmaps
return Object.prototype.toString.call(instance) === '[object WeakMap]';

This comment has been minimized.

@stefanpenner

stefanpenner Sep 10, 2016

Member

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

@stefanpenner

stefanpenner Sep 10, 2016

Member

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

This comment has been minimized.

@rwjblue

rwjblue Sep 10, 2016

Member

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

@rwjblue

rwjblue Sep 10, 2016

Member

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

This comment has been minimized.

@rwjblue

rwjblue Sep 10, 2016

Member

Looks like lodash has been through this one already:

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

rwjblue Sep 10, 2016

Member

Looks like lodash has been through this one already:

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.

This comment has been minimized.

@stefanpenner

stefanpenner Sep 10, 2016

Member

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

@stefanpenner

stefanpenner Sep 10, 2016

Member

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

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 13, 2016

Member

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?

Member

rwjblue commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 13, 2016

Member

small correction: the difference is 0.3% 😄

Member

chancancode commented Sep 13, 2016

small correction: the difference is 0.3% 😄

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 13, 2016

Member

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

Member

stefanpenner commented Sep 13, 2016

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

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 13, 2016

Member

small correction: the difference is 0.3% 😄

Derp. Thank you.

Member

rwjblue commented Sep 13, 2016

small correction: the difference is 0.3% 😄

Derp. Thank you.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 13, 2016

Member

do you want me to review the skipped tests?

@stefanpenner - Ya, absolutely if you have time.

Member

rwjblue commented Sep 13, 2016

do you want me to review the skipped tests?

@stefanpenner - Ya, absolutely if you have time.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 13, 2016

Member

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.

Member

stefanpenner commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 13, 2016

Member

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

Member

rwjblue commented Sep 13, 2016

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

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 13, 2016

Member

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

Member

stefanpenner commented Sep 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 13, 2016

Member

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

Member

rwjblue commented Sep 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 13, 2016

Member

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

Member

krisselden commented Sep 13, 2016

@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 comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 13, 2016

Member

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

Member

krisselden commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 13, 2016

Member

@rwjblue we should try ember bench on ember observer

Member

krisselden commented Sep 13, 2016

@rwjblue we should try ember bench on ember observer

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 13, 2016

Member

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.

Member

krisselden commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Sep 13, 2016

Member

@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

Member

krisselden commented Sep 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 13, 2016

Member

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

Member

stefanpenner commented Sep 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 25, 2016

Member

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.

Member

rwjblue commented Sep 25, 2016

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.

@rwjblue rwjblue merged commit 7f975bd into master Sep 25, 2016

1 check passed

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

@rwjblue rwjblue deleted the meta-via-weakmap branch Sep 25, 2016

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 26, 2016

Member

@rwjblue should we also move GUID_KEY over eventually?

Member

stefanpenner commented Sep 26, 2016

@rwjblue should we also move GUID_KEY over eventually?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 26, 2016

Member

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

Member

rwjblue commented Sep 26, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment