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

get() / set() perf optimizations #17166

Merged
merged 5 commits into from Jan 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 28 additions & 13 deletions packages/@ember/-internals/meta/lib/meta.ts
Expand Up @@ -92,7 +92,7 @@ type Listener = RemoveAllListener | StringListener | FunctionListener;
let currentListenerVersion = 1;

export class Meta {
_descriptors: any | undefined;
_descriptors: Map<string, any> | undefined;
_watching: any | undefined;
_mixins: any | undefined;
_deps: any | undefined;
Expand Down Expand Up @@ -256,6 +256,20 @@ export class Meta {
}
}

_findInheritedMap(key: string, subkey: string): any | undefined {
let pointer: Meta | null = this;
while (pointer !== null) {
let map: Map<string, any> = pointer[key];
if (map !== undefined) {
let value = map.get(subkey);
if (value !== undefined) {
return value;
}
}
pointer = pointer.parent;
}
}

_hasInInheritedSet(key: string, value: any) {
let pointer: Meta | null = this;
while (pointer !== null) {
Expand Down Expand Up @@ -461,12 +475,12 @@ export class Meta {
: '',
!this.isMetaDestroyed()
);
let map = this._getOrCreateOwnMap('_descriptors');
map[subkey] = value;
let map = this._descriptors || (this._descriptors = new Map());
map.set(subkey, value);
}

peekDescriptors(subkey: string) {
let possibleDesc = this._findInherited2('_descriptors', subkey);
let possibleDesc = this._findInheritedMap('_descriptors', subkey);
return possibleDesc === UNDEFINED ? undefined : possibleDesc;
}

Expand All @@ -480,16 +494,15 @@ export class Meta {
while (pointer !== null) {
let map = pointer._descriptors;
if (map !== undefined) {
for (let key in map) {
map.forEach((value, key) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I use Map.forEach() for the iteration, since it is the only method that works with IE11.

seen = seen === undefined ? new Set() : seen;
if (!seen.has(key)) {
seen.add(key);
let value = map[key];
if (value !== UNDEFINED) {
fn(key, value);
}
}
}
});
}
pointer = pointer.parent;
}
Expand Down Expand Up @@ -728,7 +741,7 @@ export class Meta {
return this._listeners;
}

matchingListeners(eventName: string): (string | boolean | object | null)[] | undefined | void {
matchingListeners(eventName: string): (string | boolean | object | null)[] | undefined {
let listeners = this.flattenedListeners();
let result;

Expand Down Expand Up @@ -845,7 +858,7 @@ export function setMeta(obj: object, meta: Meta) {
metaStore.set(obj, meta);
}

export function peekMeta(obj: object) {
export function peekMeta(obj: object): Meta | null {
assert('Cannot call `peekMeta` on null', obj !== null);
assert('Cannot call `peekMeta` on undefined', obj !== undefined);
assert(
Expand Down Expand Up @@ -884,6 +897,8 @@ export function peekMeta(obj: object) {

pointer = getPrototypeOf(pointer);
}

return null;
}

/**
Expand All @@ -909,7 +924,7 @@ export function deleteMeta(obj: object) {
}

let meta = peekMeta(obj);
if (meta !== undefined) {
if (meta !== null) {
meta.destroy();
}
}
Expand Down Expand Up @@ -950,7 +965,7 @@ export const meta: {
let maybeMeta = peekMeta(obj);

// remove this code, in-favor of explicit parent
if (maybeMeta !== undefined && maybeMeta.source === obj) {
if (maybeMeta !== null && maybeMeta.source === obj) {
return maybeMeta;
}

Expand All @@ -972,7 +987,7 @@ if (DEBUG) {
@return {Descriptor}
@private
*/
export function descriptorFor(obj: object, keyName: string, _meta?: Meta) {
export function descriptorFor(obj: object, keyName: string, _meta?: Meta | null) {
assert('Cannot call `descriptorFor` on null', obj !== null);
assert('Cannot call `descriptorFor` on undefined', obj !== undefined);
assert(
Expand All @@ -982,7 +997,7 @@ export function descriptorFor(obj: object, keyName: string, _meta?: Meta) {

let meta = _meta === undefined ? peekMeta(obj) : _meta;

if (meta !== undefined) {
if (meta !== null) {
return meta.peekDescriptors(keyName);
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/@ember/-internals/metal/lib/chains.ts
Expand Up @@ -8,7 +8,7 @@ function isObject(obj: any): obj is object {
return typeof obj === 'object' && obj !== null;
}

function isVolatile(obj: any, keyName: string, meta?: Meta): boolean {
function isVolatile(obj: any, keyName: string, meta?: Meta | null): boolean {
let desc = descriptorFor(obj, keyName, meta);
return !(desc !== undefined && desc._volatile === false);
}
Expand Down Expand Up @@ -117,7 +117,7 @@ function removeChainWatcher(obj: object, keyName: string, node: ChainNode, _meta
let meta = _meta === undefined ? peekMeta(obj) : _meta;

if (
meta === undefined ||
meta === null ||
meta.isSourceDestroying() ||
meta.isMetaDestroyed() ||
meta.readableChainWatchers() === undefined
Expand Down Expand Up @@ -336,7 +336,7 @@ function lazyGet(obj: object, key: string): any {
let meta = peekMeta(obj);

// check if object meant only to be a prototype
if (meta !== undefined && meta.proto === obj) {
if (meta !== null && meta.proto === obj) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/metal/lib/computed.ts
Expand Up @@ -359,7 +359,7 @@ class ComputedProperty extends Descriptor implements DescriptorWithDependentKeys

// don't create objects just to invalidate
let meta = peekMeta(obj);
if (meta === undefined || meta.source !== obj) {
if (meta === null || meta.source !== obj) {
return;
}

Expand Down
9 changes: 5 additions & 4 deletions packages/@ember/-internals/metal/lib/events.ts
Expand Up @@ -115,11 +115,12 @@ export function sendEvent(
eventName: string,
params: any[],
actions?: any[],
_meta?: Meta
): boolean {
_meta?: Meta | null
) {
if (actions === undefined) {
let meta = _meta === undefined ? peekMeta(obj) : _meta;
actions = typeof meta === 'object' && meta !== null && meta.matchingListeners(eventName);
actions =
typeof meta === 'object' && meta !== null ? meta.matchingListeners(eventName) : undefined;
}

if (actions === undefined || actions.length === 0) {
Expand Down Expand Up @@ -161,7 +162,7 @@ export function sendEvent(
*/
export function hasListeners(obj: object, eventName: string): boolean {
let meta = peekMeta(obj);
if (meta === undefined) {
if (meta === null) {
return false;
}
let matched = meta.matchingListeners(eventName);
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/metal/lib/mixin.ts
Expand Up @@ -548,7 +548,7 @@ export default class Mixin {
static mixins(obj: object): Mixin[] {
let meta = peekMeta(obj);
let ret: Mixin[] = [];
if (meta === undefined) {
if (meta === null) {
return ret;
}

Expand Down Expand Up @@ -612,7 +612,7 @@ export default class Mixin {
return _detect(obj, this);
}
let meta = peekMeta(obj);
if (meta === undefined) {
if (meta === null) {
return false;
}
return meta.hasMixin(this);
Expand Down
6 changes: 3 additions & 3 deletions packages/@ember/-internals/metal/lib/properties.ts
Expand Up @@ -63,7 +63,7 @@ interface ExtendedObject {

export function MANDATORY_SETTER_FUNCTION(name: string): MandatorySetterFunction {
function SETTER_FUNCTION(this: object, value: any | undefined | null): void {
let m = peekMeta(this);
let m = peekMeta(this)!;
if (m.isInitializing() || m.isPrototypeMeta(this)) {
m.writeValues(name, value);
} else {
Expand All @@ -79,7 +79,7 @@ export function MANDATORY_SETTER_FUNCTION(name: string): MandatorySetterFunction
export function DEFAULT_GETTER_FUNCTION(name: string): DefaultGetterFunction {
return function GETTER_FUNCTION(this: any): void {
let meta = peekMeta(this);
if (meta !== undefined) {
if (meta !== null) {
return meta.peekValues(name);
}
};
Expand All @@ -89,7 +89,7 @@ export function INHERITING_GETTER_FUNCTION(name: string): InheritingGetterFuncti
function IGETTER_FUNCTION(this: any): void {
let meta = peekMeta(this);
let val;
if (meta !== undefined) {
if (meta !== null) {
val = meta.readInheritedValue('values', name);
if (val === UNDEFINED) {
let proto = Object.getPrototypeOf(this);
Expand Down
9 changes: 4 additions & 5 deletions packages/@ember/-internals/metal/lib/property_events.ts
Expand Up @@ -34,11 +34,10 @@ let deferred = 0;
@since 3.1.0
@public
*/
function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta): void {
function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta | null): void {
let meta = _meta === undefined ? peekMeta(obj) : _meta;
let hasMeta = meta !== undefined;

if (hasMeta && (meta.isInitializing() || meta.isPrototypeMeta(obj))) {
if (meta !== null && (meta.isInitializing() || meta.isPrototypeMeta(obj))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not check once and reference it like before let hasMeta = meta !== null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the ts linter won't recognize that meta is non-null, and the subsequent statements would need a "!":

if (hasMeta && (meta!.isInitializing() || meta!.isPrototypeMeta(obj))) {

return;
}

Expand All @@ -48,7 +47,7 @@ function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta): void
possibleDesc.didChange(obj, keyName);
}

if (hasMeta && meta.peekWatching(keyName) > 0) {
if (meta !== null && meta.peekWatching(keyName) > 0) {
dependentKeysDidChange(obj, keyName, meta);
chainsDidChange(obj, keyName, meta);
notifyObservers(obj, keyName, meta);
Expand All @@ -58,7 +57,7 @@ function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta): void
obj[PROPERTY_DID_CHANGE](keyName);
}

if (hasMeta) {
if (meta !== null) {
if (meta.isSourceDestroying()) {
return;
}
Expand Down
11 changes: 6 additions & 5 deletions packages/@ember/-internals/metal/lib/property_get.ts
Expand Up @@ -96,6 +96,10 @@ export function get(obj: object, keyName: string): any {
let isFunction = type === 'function';
let isObjectLike = isObject || isFunction;

if (isPath(keyName)) {
return isObjectLike ? _getPath(obj, keyName) : undefined;
}

let value: any;

if (isObjectLike) {
Expand All @@ -119,9 +123,6 @@ export function get(obj: object, keyName: string): any {
}

if (value === undefined) {
if (isPath(keyName)) {
return _getPath(obj, keyName);
}
if (
isObject &&
!(keyName in obj) &&
Expand All @@ -133,9 +134,9 @@ export function get(obj: object, keyName: string): any {
return value;
}

export function _getPath<T extends object>(root: T, path: string): any {
export function _getPath<T extends object>(root: T, path: string | string[]): any {
let obj: any = root;
let parts = path.split('.');
let parts = typeof path === 'string' ? path.split('.') : path;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be even more performant if _getPath only accept array of path, https://github.com/emberjs/ember.js/pull/17166/files#diff-45589baa0b0954cefba717cf63c270f9R100 and here you can put isObjectLike ? _getPath(obj, keyName.split('.')) : undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about that, and you might be right. I discarded it as a micro optimization I didn't want to do in this pass. It is not likely to make any measurable difference.


for (let i = 0; i < parts.length; i++) {
if (obj === undefined || obj === null || (obj as MaybeHasIsDestroyed).isDestroyed) {
Expand Down
15 changes: 8 additions & 7 deletions packages/@ember/-internals/metal/lib/property_set.ts
Expand Up @@ -14,7 +14,7 @@ interface ExtendedObject {
}

let setWithMandatorySetter: <T extends object, K extends Extract<keyof T, string>>(
meta: Meta,
meta: Meta | null,
obj: T,
keyName: K,
value: T[K]
Expand Down Expand Up @@ -116,7 +116,7 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean

if (DEBUG) {
setWithMandatorySetter = (meta, obj, keyName, value) => {
if (meta !== undefined && meta.peekWatching(keyName) > 0) {
if (meta !== null && meta.peekWatching(keyName) > 0) {
makeEnumerable(obj, keyName);
meta.writeValue(obj, keyName, value);
} else {
Expand All @@ -136,17 +136,18 @@ if (DEBUG) {

function setPath(root: object, path: string, value: any, tolerant?: boolean): any {
let parts = path.split('.');
let keyName = parts.pop() as string;
let keyName = parts.pop()!;

assert('Property set failed: You passed an empty path', keyName!.trim().length > 0);
assert('Property set failed: You passed an empty path', keyName.trim().length > 0);

let newPath = parts.join('.');
let newRoot = getPath(root, newPath);
let newRoot = getPath(root, parts);

if (newRoot !== null && newRoot !== undefined) {
return set(newRoot, keyName, value);
} else if (!tolerant) {
throw new EmberError(`Property set failed: object in path "${newPath}" could not be found.`);
throw new EmberError(
`Property set failed: object in path "${parts.join('.')}" could not be found.`
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/metal/lib/watch_key.ts
Expand Up @@ -94,7 +94,7 @@ export function unwatchKey(obj: object, keyName: string, _meta?: Meta): void {
let meta = _meta === undefined ? peekMeta(obj) : _meta;

// do nothing of this object has already been destroyed
if (meta === undefined || meta.isSourceDestroyed()) {
if (meta === null || meta.isSourceDestroyed()) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/metal/lib/watch_path.ts
Expand Up @@ -14,7 +14,7 @@ export function watchPath(obj: any, keyPath: string, meta?: Meta): void {

export function unwatchPath(obj: any, keyPath: string, meta?: Meta): void {
let m = meta === undefined ? peekMeta(obj) : meta;
if (m === undefined) {
if (m === null) {
return;
}
let counter = m.peekWatching(keyPath);
Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/metal/lib/watching.ts
Expand Up @@ -33,7 +33,7 @@ export function isWatching(obj: any, key: string): boolean {

export function watcherCount(obj: any, key: string): number {
let meta = peekMeta(obj);
return (meta !== undefined && meta.peekWatching(key)) || 0;
return (meta !== null && meta.peekWatching(key)) || 0;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/ember/tests/routing/query_params_test.js
Expand Up @@ -1334,7 +1334,7 @@ moduleFor(
Route.extend({
model(p, trans) {
let m = peekMeta(trans[PARAMS_SYMBOL].application);
assert.ok(m === undefined, "A meta object isn't constructed for this params POJO");
assert.ok(m === null, "A meta object isn't constructed for this params POJO");
},
})
);
Expand Down