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

[BUGFIX canary] Meta refactor #11966

Merged
merged 40 commits into from
Aug 5, 2015
Merged

[BUGFIX canary] Meta refactor #11966

merged 40 commits into from
Aug 5, 2015

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Aug 3, 2015

This refactors the meta object as a step toward optimizing it better.

  • consolidates and encapsulates meta's data structures so we can more easily replace them
  • switches to explicit inheritance between meta objects over prototypical inheritance, so we can pick and choose how each member is actually inherited, and choose to do optimizations.
  • refactors meta.listeners in an attempt to make listener creation and deletion cheaper, especially in terms of allocations. This is still an early sketch and more effort is needed to see if it's actually a good idea.

Meta.prototype['writable' + capitalized] = function(create) {
let ret = this[key];
if (!ret) {
ret = this[key] = create(this.source);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can do this in a quicker way, as long as we are aware which are for instances and which are for classes.

basically, for an instance if we could do

ret = this[key] = new InheritedObjectSubclass();

for classes we would do something like:

function InheritedObjectSubclass() {

}

InheritedObjectSubclass.prototype = create(this.source);

we would still have 1 shape per InheritedObjectSubclass (which still will likely cause grief), but we could fast-path the instantiation for instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Right now there is a single shape at every level, but you're saying it may be worth it to accept a shape-per-class in order to make instance creation faster, since presumably there are so many more instances than classes.

I guess we would need to try and measure the impact, I am uncertain. Deoptimizing some key method due to multiple-shapeness could be way worth than the instance instantiation cost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you're saying it may be worth it to accept a shape-per-class in order to make instance creation faster

You know, if we do the readable/writable thing correctly, the above likely doesn't matter. As we will now only lazily create the children. In all common cases, I hope that means that we just wont incur the cost of the children at all.

We may want to add logging to detect when this assumption is violated.

We can keep it up our sleeves, but after reading the full PR, I believe wont provide any value.

@ef4 ef4 changed the title [WIP] Meta refactor [BUGFIX canary] Meta refactor Aug 4, 2015
@ef4
Copy link
Contributor Author

ef4 commented Aug 4, 2015

I think we should merge this, so further work can be based off it.

I would also like to get perf feedback from apps on canary, to confirm that we are at least not making anything worse by accident.

@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2015

Restarted sauce labs job.

@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2015

I'm 👍 on landing this in canary.

@stefanpenner
Copy link
Member

I would also like to get perf feedback from apps on canary, to confirm that we are at least not making anything worse by accident.

Let me get some time this afternoon to run this in some apps, just want to do our due diligence.

}
});

function EmptyObject() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly more concise version is possible

export default function EmptyObject() {

}

EmptyObject.prototype = null

6ms -> noise

turns out there is no 0 cost abstraction in JS yet...
@stefanpenner
Copy link
Member

something of interest, i have added some ghetto instrumentation to get a picture of how the new code is being utilized:

_createOwnMap: 3008
_geInherited: 25355
_getOrCreateInheritedMap: 17289
_getOrCreateInheritedMap.create: 110
_getOrCreateInheritedMap.fromParent: 5287
_getOrCreateOwnMap: 5269
inheritedCustomObject_readableChains: 2220
inheritedCustomObject_writableChains: 0
inheritedMapOfMaps_getAllDeps: 0
inheritedMapOfMaps_readableDeps: 6
inheritedMapOfMaps_writableDeps: 4404
inheritedMap_clearBindings: 0
inheritedMap_clearMixins: 0
inheritedMap_clearValues: 0
inheritedMap_clearWatching: 0
inheritedMap_peekBindings: 0
inheritedMap_peekMixins: 5405
inheritedMap_peekValues: 0
inheritedMap_peekWatching: 15367
inheritedMap_readAbleBindings: 2357
inheritedMap_readAbleMixins: 0
inheritedMap_readAbleValues: 0
inheritedMap_readAbleWatching: 0
inheritedMap_writableBindings: 0
inheritedMap_writableMixins: 188
inheritedMap_writableValues: 0
inheritedMap_writableWatching: 7410
ownCustomObject_readableChainWatchers: 7438
ownCustomObject_writableChainWatchers: 0
ownMap_reableCache: 28
ownMap_writable_Cache: 5269

the following is likely the worse offender in-terms of performance.

_getOrCreateInheritedMap.fromParent: 5287

it appears the most costly, are listeners to descriptors that likely never change (at least not in this example). In addition, these listeners could actually be inherited listeners, as the CP in question are defined on the classes, not the instances.

It seems like an interesting optimization to pursue at a future point in time.

stefanpenner added a commit that referenced this pull request Aug 5, 2015
[BUGFIX canary] Meta refactor
@stefanpenner stefanpenner merged commit 41ece2b into emberjs:master Aug 5, 2015
@stefanpenner
Copy link
Member

this is a great start in cleaning up our internals. Good job @ef4, this will be nice to work on-top of for our next opts.

}
});

QUnit.test('meta is not enumerable', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just reading through this PR out of curiosity. There may be a reason I don't understand, but this test seem to be repeated twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, seems like a dupe. Mind submitting a PR to remove the duplicate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants