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

[BACKPORT] Performance Tuning for Ember 3.13 #963

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@
"files.trimTrailingWhitespace": true,
"editor.renderWhitespace": "boundary",
"editor.insertSpaces": true,
"editor.tabSize": 2
"editor.tabSize": 2,
"workbench.colorCustomizations": {}
}
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@
"tslint-config-prettier": "^1.12.0",
"tslint-plugin-prettier": "^1.3.0"
},
"resolutions": {
"typescript": "3.3.3",
"broccoli-typescript-transpiler/typescript": "3.3.3"
},
"changelog": {
"repo": "glimmerjs/glimmer-vm",
"labels": {
Expand All @@ -91,4 +95,4 @@
"build/symlink-dependencies"
]
}
}
}
21 changes: 15 additions & 6 deletions packages/@glimmer/reference/index.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
export { Reference as BasicReference, PathReference as BasicPathReference } from './lib/reference';
export {
Reference as BasicReference,
PathReference as BasicPathReference,
VersionedReference as Reference,
VersionedPathReference as PathReference,
VersionedReference,
VersionedPathReference,
CachedReference,
Mapper,
map,
ReferenceCache,
Validation,
NotModified,
isModified,
} from './lib/reference';

export { ConstReference } from './lib/const';

export { ListItem } from './lib/iterable';

export * from './lib/validators';

export {
VersionedReference as Reference,
VersionedPathReference as PathReference,
} from './lib/validators';

export {
IterationItem,
Iterator,
Expand Down
3 changes: 2 additions & 1 deletion packages/@glimmer/reference/lib/const.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { CONSTANT_TAG, VersionedReference, Tag } from './validators';
import { CONSTANT_TAG, Tag } from './validators';
import { VersionedReference } from './reference';

export class ConstReference<T> implements VersionedReference<T> {
public tag: Tag = CONSTANT_TAG;
Expand Down
3 changes: 2 additions & 1 deletion packages/@glimmer/reference/lib/iterable.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { LinkedList, ListNode, Opaque, Option, dict, expect } from '@glimmer/util';
import { VersionedPathReference as PathReference, Tag } from './validators';
import { Tag } from './validators';
import { VersionedPathReference as PathReference } from './reference';

export interface IterationItem<T, U> {
key: string;
Expand Down
128 changes: 127 additions & 1 deletion packages/@glimmer/reference/lib/reference.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Opaque } from '@glimmer/util';
import { Opaque, Option } from '@glimmer/util';
import { Revision, Tag, Tagged, value, validate } from './validators';

export interface Reference<T> {
value(): T;
Expand All @@ -9,3 +10,128 @@ export default Reference;
export interface PathReference<T> extends Reference<T> {
get(key: string): PathReference<Opaque>;
}

//////////

export interface VersionedReference<T = Opaque> extends Reference<T>, Tagged {}

export interface VersionedPathReference<T = Opaque> extends PathReference<T>, Tagged {
get(property: string): VersionedPathReference<Opaque>;
}

export abstract class CachedReference<T> implements VersionedReference<T> {
public abstract tag: Tag;

private lastRevision: Option<Revision> = null;
private lastValue: Option<T> = null;

value(): T {
let { tag, lastRevision, lastValue } = this;

if (lastRevision === null || !validate(tag, lastRevision)) {
lastValue = this.lastValue = this.compute();
this.lastRevision = value(tag);
}

return lastValue as T;
}

protected abstract compute(): T;

protected invalidate() {
this.lastRevision = null;
}
}

//////////

export type Mapper<T, U> = (value: T) => U;

class MapperReference<T, U> extends CachedReference<U> {
public tag: Tag;

private reference: VersionedReference<T>;
private mapper: Mapper<T, U>;

constructor(reference: VersionedReference<T>, mapper: Mapper<T, U>) {
super();
this.tag = reference.tag;
this.reference = reference;
this.mapper = mapper;
}

protected compute(): U {
let { reference, mapper } = this;
return mapper(reference.value());
}
}

export function map<T, U>(
reference: VersionedReference<T>,
mapper: Mapper<T, U>
): VersionedReference<U> {
return new MapperReference<T, U>(reference, mapper);
}

//////////

export class ReferenceCache<T> implements Tagged {
public tag: Tag;

private reference: VersionedReference<T>;
private lastValue: Option<T> = null;
private lastRevision: Option<Revision> = null;
private initialized = false;

constructor(reference: VersionedReference<T>) {
this.tag = reference.tag;
this.reference = reference;
}

peek(): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

why does peek() force it compute? that smells a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pzuraq What's up here?

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 code was also ported over without changes from the validators.ts file. I agree it probably doesn't make sense to force a compute from a method called peek, but I do think that's outside of the scope of this PR/change, happy to address it in a followup though when we start cleaning some stuff up

if (!this.initialized) {
return this.initialize();
}

return this.lastValue as T;
}

revalidate(): Validation<T> {
if (!this.initialized) {
return this.initialize();
}

let { reference, lastRevision } = this;
let tag = reference.tag;

if (validate(tag, lastRevision as number)) return NOT_MODIFIED;
this.lastRevision = value(tag);

let { lastValue } = this;
let currentValue = reference.value();
if (currentValue === lastValue) return NOT_MODIFIED;
this.lastValue = currentValue;

return currentValue;
}

private initialize(): T {
let { reference } = this;

let currentValue = (this.lastValue = reference.value());
this.lastRevision = value(reference.tag);
this.initialized = true;

return currentValue;
}
}

export type Validation<T> = T | NotModified;

export type NotModified = 'adb3b78e-3d22-4e4b-877a-6317c2c5c145';

const NOT_MODIFIED: NotModified = 'adb3b78e-3d22-4e4b-877a-6317c2c5c145';

export function isModified<T>(value: Validation<T>): value is T {
return value !== NOT_MODIFIED;
Copy link
Contributor

Choose a reason for hiding this comment

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

String equality is a little weird here. why not make NOT_MODIFIED a Symbol('NOT_MODIFIED')?

If we are trying to avoid symbols, you are only using === for a unique value, you can just make an {} and then type it as something unique.

interface NotModified { __brand__: 'adb3b78e-3d22-4e4b-877a-6317c2c5c145' }
const NotModified: NotModified = {} as any;

export function isModified<T>(value: T | NotModified): value is T {
    return value !== NotModified;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair, this code is just moved from the validators file. I think it’d make sense to update it in a follow up 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking, using a string as a fake symbol allows you to both use the value as a key in an object, as well as cheaply/easily comparing it. I would generally prefer to use symbols as unique tokens, but until we can, I prefer long strings containing UUIDs.

}
Loading