-
Notifications
You must be signed in to change notification settings - Fork 191
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
Changes from all commits
49dc074
6f3680a
84afef7
1c0a90c
599467e
781f586
301b3b5
3633ac6
4f0ae53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
@@ -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 { | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 calledpeek
, 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