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.

Sign up for GitHub

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

Cleanup Meta Descs #12852

Closed
wants to merge 1 commit into from
Closed

Cleanup Meta Descs #12852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/ember-metal/lib/chains.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ function lazyGet(obj, key) {
}

// Use `get` if the return value is an EachProxy or an uncacheable value.
if (isVolatile(obj[key])) {
if (isVolatile(meta.peekDescs(key))) {
return get(obj, key);
// Otherwise attempt to get the cached value of the computed property
} else {
Expand Down
3 changes: 1 addition & 2 deletions packages/ember-metal/lib/computed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,7 @@ ComputedPropertyPrototype._throwReadOnlyError = function computedPropertyThrowRe
};

ComputedPropertyPrototype.clobberSet = function computedPropertyClobberSet(obj, keyName, value) {
let cachedValue = cacheFor(obj, keyName);
defineProperty(obj, keyName, null, cachedValue);
defineProperty(obj, keyName, null, cacheFor(obj, keyName));
set(obj, keyName, value);
return value;
};
Expand Down
7 changes: 3 additions & 4 deletions packages/ember-metal/lib/injected_property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { ComputedProperty } from 'ember-metal/computed';
import { AliasedProperty } from 'ember-metal/alias';
import { Descriptor } from 'ember-metal/properties';
import { getOwner } from 'container/owner';
import { meta } from 'ember-metal/meta';

/**
Read-only property that returns the result of a container lookup.
Expand All @@ -15,7 +16,7 @@ import { getOwner } from 'container/owner';
to the property's name
@private
*/
function InjectedProperty(type, name) {
export default function InjectedProperty(type, name) {
this.type = type;
this.name = name;

Expand All @@ -24,7 +25,7 @@ function InjectedProperty(type, name) {
}

function injectedPropertyGet(keyName) {
var desc = this[keyName];
var desc = meta(this).peekDescs(keyName);
let owner = getOwner(this);

assert(`InjectedProperties should be defined with the Ember.inject computed property macros.`, desc && desc.isDescriptor && desc.type);
Copy link
Member Author

Choose a reason for hiding this comment

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

isDescriptor check isn't required

Expand All @@ -45,5 +46,3 @@ InjectedPropertyPrototype.get = ComputedPropertyPrototype.get;
InjectedPropertyPrototype.readOnly = ComputedPropertyPrototype.readOnly;

InjectedPropertyPrototype.teardown = ComputedPropertyPrototype.teardown;

export default InjectedProperty;
2 changes: 2 additions & 0 deletions packages/ember-metal/lib/meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import EmptyObject from 'ember-metal/empty_object';
let members = {
cache: ownMap,
weak: ownMap,
descs: inheritedMap,
watching: inheritedMap,
mixins: inheritedMap,
bindings: inheritedMap,
Expand All @@ -57,6 +58,7 @@ function Meta(obj, parentMeta) {
this._chainWatchers = undefined;
this._chains = undefined;
this._tag = undefined;
this._descs = undefined;

// used only internally
this.source = obj;
Expand Down
14 changes: 6 additions & 8 deletions packages/ember-metal/lib/mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,7 @@ function giveDescriptorSuper(meta, key, property, values, descs, base) {
// If we didn't find the original descriptor in a parent mixin, find
// it on the original object.
if (!superProperty) {
var possibleDesc = base[key];
var superDesc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;

superProperty = superDesc;
superProperty = meta.peekDescs(key);
}

if (superProperty === undefined || !(superProperty instanceof ComputedProperty)) {
Expand Down Expand Up @@ -235,7 +232,7 @@ function mergeMixins(mixins, m, descs, values, base, keys) {
currentMixin = mixins[i];
assert(
`Expected hash or Mixin instance, got ${Object.prototype.toString.call(currentMixin)}`,
typeof currentMixin === 'object' && currentMixin !== null && Object.prototype.toString.call(currentMixin) !== '[object Array]'
typeof currentMixin === 'object' && currentMixin !== null && !Array.isArray(currentMixin)
);

props = mixinProperties(m, currentMixin);
Expand Down Expand Up @@ -329,7 +326,7 @@ function followAlias(obj, desc, m, descs, values) {
if (descs[altKey] || values[altKey]) {
value = values[altKey];
desc = descs[altKey];
} else if ((possibleDesc = obj[altKey]) && possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) {
} else if (possibleDesc = m.peekDescs(altKey)) {
desc = possibleDesc;
value = undefined;
} else {
Expand All @@ -351,7 +348,8 @@ function updateObserversAndListeners(obj, key, observerOrListener, pathsKey, upd
}

function replaceObserversAndListeners(obj, key, observerOrListener) {
var prev = obj[key];
var desc = Object.getOwnPropertyDescriptor(obj, key);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can handle this more performently, but might require either extra branching or a re-think

var prev = desc && desc.value;

if ('function' === typeof prev) {
updateObserversAndListeners(obj, key, prev, '__ember_observesBefore__', _removeBeforeObserver);
Expand Down Expand Up @@ -386,6 +384,7 @@ function applyMixin(obj, mixins, partial) {

for (var i = 0, l = keys.length; i < l; i++) {
key = keys[i];

if (key === 'constructor' || !values.hasOwnProperty(key)) { continue; }

desc = descs[key];
Expand All @@ -400,7 +399,6 @@ function applyMixin(obj, mixins, partial) {
}

if (desc === undefined && value === undefined) { continue; }

replaceObserversAndListeners(obj, key, value);
detectBinding(obj, key, value, m);
defineProperty(obj, key, desc, value, m);
Expand Down
68 changes: 42 additions & 26 deletions packages/ember-metal/lib/properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,40 +112,54 @@ export function INHERITING_GETTER_FUNCTION(name) {
@param {*} [data] something other than a descriptor, that will
become the explicit value of this property.
*/
export function defineProperty(obj, keyName, desc, data, meta) {
var possibleDesc, existingDesc, watching, value;
export function defineProperty(obj, keyName, desc, data/*, meta*/) {
let meta = arguments[4] || metaFor(obj);
Copy link
Member Author

Choose a reason for hiding this comment

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

a meta may not exists here yet, we should be careful to not create on if it does not need to exist.

let watchEntry = meta.peekWatching(keyName);
let existingDesc = meta.peekDescs(keyName);

if (!meta) {
meta = metaFor(obj);
}
var watchEntry = meta.peekWatching(keyName);
possibleDesc = obj[keyName];
existingDesc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;

watching = watchEntry !== undefined && watchEntry > 0;
let watching = watchEntry !== undefined && watchEntry > 0;

if (existingDesc) {
existingDesc.teardown(obj, keyName);
}

let value;

// is this instanceof needed?
if (desc instanceof Descriptor) {
value = desc;
if (isEnabled('mandatory-setter')) {
if (watching) {
Object.defineProperty(obj, keyName, {
configurable: true,
enumerable: true,
writable: true,
value: value
});
} else {
meta.writeDescs(keyName, desc);
Copy link
Member Author

Choose a reason for hiding this comment

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

we should likely extract this algo into installDescriptor function or something


Object.defineProperty(obj, keyName, {
configurable: true,
enumerable: true,
get() {
return metaFor(this).peekDescs(keyName).get(this, keyName);
},
set(value) {
// throw TypeError('Sorry this isnt supported "right now"');
Copy link
Member Author

Choose a reason for hiding this comment

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

decide what we want to do here. (complex topic that will need more research)

delete obj[keyName];
obj[keyName] = value;
}
} else {
obj[keyName] = value;
}
});

// if (isEnabled('mandatory-setter')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

dealing with this will depend on the outcome of https://github.com/emberjs/ember.js/pull/12852/files#r50556996

// if (watching) {
// Object.defineProperty(obj, keyName, {
// configurable: true,
// enumerable: true,
// writable: true,
// value: value
// });
// } else {
// obj[keyName] = value;
// }
// } else {
// obj[keyName] = value;
// }
if (desc.setup) { desc.setup(obj, keyName); }
} else {
// add insert undefined subroutine, to do this more efficiently
meta.writeDescs(keyName, false);
Copy link
Member Author

Choose a reason for hiding this comment

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

need a more stable way to insert "no such member" at this level of the prototype.

if (desc == null) {
value = data;

Expand All @@ -169,11 +183,13 @@ export function defineProperty(obj, keyName, desc, data, meta) {
obj[keyName] = data;
}
} else {
obj[keyName] = data;
Object.defineProperty(obj, keyName, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be nice if we could avoid this, but we do stop on CP's sometimes, which requires us to remove the existing descriptor. A likely approach is to check if this property has a desc or not, then decide defProp or not.

(defProp tends to currently have much worse performance then a regular prop set)

configurable: true,
writable: true,
value: data
});
}
} else {
value = desc;

// fallback to ES5
Object.defineProperty(obj, keyName, desc);
}
Expand Down
14 changes: 5 additions & 9 deletions packages/ember-metal/lib/property_events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ function propertyWillChange(obj, keyName) {
var m = peekMeta(obj);
var watching = (m && m.peekWatching(keyName) > 0) || keyName === 'length';
var proto = m && m.proto;
var possibleDesc = obj[keyName];
var desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;
var desc = m && m.peekDescs(keyName);

if (!watching) {
return;
Expand Down Expand Up @@ -81,8 +80,7 @@ function propertyDidChange(obj, keyName) {
var m = peekMeta(obj);
var watching = (m && m.peekWatching(keyName) > 0) || keyName === 'length';
var proto = m && m.proto;
var possibleDesc = obj[keyName];
var desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;
var desc = m && m.peekDescs(keyName);

if (proto === obj) {
return;
Expand Down Expand Up @@ -151,9 +149,8 @@ function dependentKeysDidChange(obj, depKey, meta) {
}

function iterDeps(method, obj, depKey, seen, meta) {
var possibleDesc, desc;
var guid = guidFor(obj);
var current = seen[guid];
let guid = guidFor(obj);
let current = seen[guid];

if (!current) {
current = seen[guid] = {};
Expand All @@ -168,8 +165,7 @@ function iterDeps(method, obj, depKey, seen, meta) {
meta.forEachInDeps(depKey, (key, value) => {
if (!value) { return; }

possibleDesc = obj[key];
desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;
let desc = meta.peekDescs(key);

if (desc && desc._suspended === obj) {
return;
Expand Down
22 changes: 7 additions & 15 deletions packages/ember-metal/lib/property_get.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,26 +48,18 @@ export function get(obj, keyName) {
return obj;
}

var value = obj[keyName];
var desc = (value !== null && typeof value === 'object' && value.isDescriptor) ? value : undefined;
var ret;

if (desc === undefined && isPath(keyName)) {
if (isPath(keyName)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

needs testing to be sure checking descs isn't cheaper then this. but keeping it simpler at first is likely better, but we should investigate further.

return _getPath(obj, keyName);
}

if (desc) {
return desc.get(obj, keyName);
} else {
ret = value;

if (ret === undefined &&
'object' === typeof obj && !(keyName in obj) && 'function' === typeof obj.unknownProperty) {
return obj.unknownProperty(keyName);
}
let ret = obj[keyName];

return ret;
if (ret === undefined &&
'object' === typeof obj && !(keyName in obj) && 'function' === typeof obj.unknownProperty) {
return obj.unknownProperty(keyName);
}

return ret;
}

export function _getPath(root, path) {
Expand Down
10 changes: 5 additions & 5 deletions packages/ember-metal/lib/property_set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,23 @@ export function set(obj, keyName, value, tolerant) {
assert(`The key provided to set must be a string, you passed ${keyName}`, typeof keyName === 'string');
assert(`'this' in paths is not supported`, !pathHasThis(keyName));

let meta, possibleDesc, desc;

let meta, desc;
if (obj) {
meta = peekMeta(obj);
possibleDesc = obj[keyName];
desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined;
desc = meta && meta.peekDescs(keyName);
markObjectAsDirty(meta);
}

var isUnknown, currentValue;
let isUnknown, currentValue;
if (desc === undefined && isPath(keyName)) {
return setPath(obj, keyName, value, tolerant);
}

assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`,
!obj.isDestroyed);


// soon this will be just obj[keyName] = value, as it shouldn be...
Copy link
Member Author

Choose a reason for hiding this comment

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

this will depend on the outcome of https://github.com/emberjs/ember.js/pull/12852/files#r50556996

if (desc) {
desc.set(obj, keyName, value);
} else {
Expand Down
Loading