[BACKPORT] Performance Tuning for Ember 3.13 #963
Conversation
|
|
||
| if (tag === VOLATILE_TAG) return VOLATILE_TAG; | ||
| if (tag !== CONSTANT_TAG) optimized.push(tag); | ||
| [TYPE]: MonomorphicTagType; |
There was a problem hiding this comment.
I may just be not understanding what the actual MonomorphicTag type is, but why do you have to use the brands here instead of the const enum?
There was a problem hiding this comment.
It's because TYPE is the intersection of all of the types on all of the classes that make up the MonomorphicTag type (it being the intersection of all the subtypes), which is slightly different than the actual enum. This is part of what gets us the ability to magically do new MonomorhpicTagImpl(TagType.Dirtyable) and have that type to the DirtyableTag interface.
@krisselden may be able to explain better, this was his dark type magic that I'm still trying to wrap my head around 😄
pzuraq
force-pushed
the
backport/updatable-dirtyable-tag
branch
2 times, most recently
from
bc73ded
to
3b96eaf
Compare
pzuraq
force-pushed
the
backport/updatable-dirtyable-tag
branch
from
3b96eaf
to
3633ac6
Compare
|
@pzuraq - Where are we at this this? |
| this.reference = reference; | ||
| } | ||
|
|
||
| peek(): T { |
There was a problem hiding this comment.
why does peek() force it compute? that smells a bit.
There was a problem hiding this comment.
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
| 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.
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;
}
There was a problem hiding this comment.
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.
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.
|
I'm pressing the merge button but could use a response to Kris' question. |
|
@pzuraq - Can you make a PR for this against master? I don't want us to loose these optimizations when we do finally update Ember to master of glimmer-vm... |
This PR represents the Glimmer portion of emberjs/ember.js#18223