Skip to content

Commit

Permalink
Use _private instead of #private properties
Browse files Browse the repository at this point in the history 
The updates to the template compiler made in support of strict mode
introduced a number of uses of private class fields, some of them in
hot paths. Since `tsc` currently transpiles private class fields to
`WeakMap`s for all values of `target` lower than `ESNEXT`, this results
in dramatically higher memory usage and garbage collection than in the
previous version of the template compiler. This in turn causes at least
a large portion of the regression noted in [emberjs/ember.js#19750][1].

[1]: emberjs/ember.js#19750

Here, we replace *all* private class fields with `_` private fields,
which substantially (thought not 100%) closed the gap with the original.

Co-authored-by: Brenden Palmer <[email protected]>
  • Loading branch information
@chriskrycho @brendenpalmer
chriskrycho and brendenpalmer committed Sep 10, 2021
1 parent 56f5c1e commit c1ec710
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 58 deletions.
16 changes: 8 additions & 8 deletions packages/@glimmer/compiler/lib/passes/1-normalization/context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,29 @@ import { VISIT_STMTS } from './visitors/statements';
* This is the mutable state for this compiler pass.
*/
export class NormalizationState {
#currentScope: SymbolTable;
#cursorCount = 0;
_currentScope: SymbolTable;
_cursorCount = 0;

constructor(block: SymbolTable, readonly isStrict: boolean) {
this.#currentScope = block;
this._currentScope = block;
}

generateUniqueCursor(): string {
return `%cursor:${this.#cursorCount++}%`;
return `%cursor:${this._cursorCount++}%`;
}

get scope(): SymbolTable {
return this.#currentScope;
return this._currentScope;
}

visitBlock(block: ASTv2.Block): Result<OptionalList<mir.Statement>> {
let oldBlock = this.#currentScope;
this.#currentScope = block.scope;
let oldBlock = this._currentScope;
this._currentScope = block.scope;

try {
return VISIT_STMTS.visitList(block.body, this);
} finally {
this.#currentScope = oldBlock;
this._currentScope = oldBlock;
}
}
}
12 changes: 6 additions & 6 deletions packages/@glimmer/compiler/lib/passes/1-normalization/keywords/impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,18 @@ function getCalleeExpression(

export class Keywords<K extends KeywordType, KeywordList extends Keyword<K> = never>
implements Keyword<K, OutFor<KeywordList>> {
#keywords: Keyword[] = [];
#type: K;
_keywords: Keyword[] = [];
_type: K;

constructor(type: K) {
this.#type = type;
this._type = type;
}

kw<S extends string = string, Out = unknown>(
name: S,
delegate: KeywordDelegate<KeywordMatches[K], unknown, Out>
): Keywords<K, KeywordList | Keyword<K, Out>> {
this.#keywords.push(keyword(name, this.#type, delegate));
this._keywords.push(keyword(name, this._type, delegate));

return this;
}
Expand All @@ -188,7 +188,7 @@ export class Keywords<K extends KeywordType, KeywordList extends Keyword<K> = ne
node: KeywordCandidates[K],
state: NormalizationState
): Result<OutFor<KeywordList>> | null {
for (let keyword of this.#keywords) {
for (let keyword of this._keywords) {
let result = keyword.translate(node, state) as Result<OutFor<KeywordList>>;
if (result !== null) {
return result;
Expand All @@ -200,7 +200,7 @@ export class Keywords<K extends KeywordType, KeywordList extends Keyword<K> = ne
if (path && path.type === 'Path' && path.ref.type === 'Free' && isKeyword(path.ref.name)) {
let { name } = path.ref;

let usedType = this.#type;
let usedType = this._type;
let validTypes = KEYWORDS_TYPES[name];

if (validTypes.indexOf(usedType) === -1) {
Expand Down
24 changes: 12 additions & 12 deletions packages/@glimmer/syntax/lib/source/loc/match.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ export type IsInvisible = 'IS_INVISIBLE';
type Pattern = OffsetKind | IsInvisible | MatchAny;

class WhenList<Out> {
#whens: When<Out>[];
_whens: When<Out>[];

constructor(whens: When<Out>[]) {
this.#whens = whens;
this._whens = whens;
}

first(kind: OffsetKind): Out | null {
for (let when of this.#whens) {
for (let when of this._whens) {
let value = when.match(kind);
if (isPresent(value)) {
return value[0];
Expand All @@ -47,33 +47,33 @@ class WhenList<Out> {
}

class When<Out> {
#map: Map<Pattern, Out> = new Map();
_map: Map<Pattern, Out> = new Map();

get(pattern: Pattern, or: () => Out): Out {
let value = this.#map.get(pattern);
let value = this._map.get(pattern);

if (value) {
return value;
}

value = or();

this.#map.set(pattern, value);
this._map.set(pattern, value);

return value;
}

add(pattern: Pattern, out: Out): void {
this.#map.set(pattern, out);
this._map.set(pattern, out);
}

match(kind: OffsetKind): Out[] {
let pattern = patternFor(kind);

let out: Out[] = [];

let exact = this.#map.get(pattern);
let fallback = this.#map.get(MatchAny);
let exact = this._map.get(pattern);
let fallback = this._map.get(MatchAny);

if (exact) {
out.push(exact);
Expand Down Expand Up @@ -105,7 +105,7 @@ export function match<Out>(callback: (m: Matcher<Out>) => ExhaustiveMatcher<Out>
}

class Matcher<Out, M extends Matches = Matches> {
#whens: When<When<(left: PositionData, right: PositionData) => Out>> = new When();
_whens: When<When<(left: PositionData, right: PositionData) => Out>> = new When();

/**
* You didn't exhaustively match all possibilities.
Expand All @@ -118,7 +118,7 @@ class Matcher<Out, M extends Matches = Matches> {
left: OffsetKind,
right: OffsetKind
): (left: PositionData, right: PositionData) => Out {
let nesteds = this.#whens.match(left);
let nesteds = this._whens.match(left);

assert(
isPresent(nesteds),
Expand Down Expand Up @@ -179,7 +179,7 @@ class Matcher<Out, M extends Matches = Matches> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
callback: (left: any, right: any) => Out
): Matcher<Out, Matches> | ExhaustiveMatcher<Out> {
this.#whens.get(left, () => new When()).add(right, callback);
this._whens.get(left, () => new When()).add(right, callback);

return this;
}
Expand Down
18 changes: 9 additions & 9 deletions packages/@glimmer/syntax/lib/source/loc/offset.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export class CharPosition implements PositionData {
readonly kind = OffsetKind.CharPosition;

/** Computed from char offset */
#locPos: HbsPosition | BROKEN | null = null;
_locPos: HbsPosition | BROKEN | null = null;

constructor(readonly source: Source, readonly charPos: number) {}

Expand Down Expand Up @@ -206,15 +206,15 @@ export class CharPosition implements PositionData {
* computing the `HbsPosition` should be a one-time operation.
*/
toHbsPos(): HbsPosition | null {
let locPos = this.#locPos;
let locPos = this._locPos;

if (locPos === null) {
let hbsPos = this.source.hbsPosFor(this.charPos);

if (hbsPos === null) {
this.#locPos = locPos = BROKEN;
this._locPos = locPos = BROKEN;
} else {
this.#locPos = locPos = new HbsPosition(this.source, hbsPos, this.charPos);
this._locPos = locPos = new HbsPosition(this.source, hbsPos, this.charPos);
}
}

Expand All @@ -225,14 +225,14 @@ export class CharPosition implements PositionData {
export class HbsPosition implements PositionData {
readonly kind = OffsetKind.HbsPosition;

#charPos: CharPosition | BROKEN | null;
_charPos: CharPosition | BROKEN | null;

constructor(
readonly source: Source,
readonly hbsPos: SourcePosition,
charPos: number | null = null
) {
this.#charPos = charPos === null ? null : new CharPosition(source, charPos);
this._charPos = charPos === null ? null : new CharPosition(source, charPos);
}

/**
Expand All @@ -244,15 +244,15 @@ export class HbsPosition implements PositionData {
* @implements {PositionData}
*/
toCharPos(): CharPosition | null {
let charPos = this.#charPos;
let charPos = this._charPos;

if (charPos === null) {
let charPosNumber = this.source.charPosFor(this.hbsPos);

if (charPosNumber === null) {
this.#charPos = charPos = BROKEN;
this._charPos = charPos = BROKEN;
} else {
this.#charPos = charPos = new CharPosition(this.source, charPosNumber);
this._charPos = charPos = new CharPosition(this.source, charPosNumber);
}
}

Expand Down
28 changes: 14 additions & 14 deletions packages/@glimmer/syntax/lib/source/loc/span.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ type AnySpan = HbsSpan | CharPositionSpan | InvisibleSpan;
class CharPositionSpan implements SpanData {
readonly kind = OffsetKind.CharPosition;

#locPosSpan: HbsSpan | BROKEN | null = null;
_locPosSpan: HbsSpan | BROKEN | null = null;

constructor(
readonly source: Source,
Expand Down Expand Up @@ -350,16 +350,16 @@ class CharPositionSpan implements SpanData {
}

toHbsSpan(): HbsSpan | null {
let locPosSpan = this.#locPosSpan;
let locPosSpan = this._locPosSpan;

if (locPosSpan === null) {
let start = this.charPositions.start.toHbsPos();
let end = this.charPositions.end.toHbsPos();

if (start === null || end === null) {
locPosSpan = this.#locPosSpan = BROKEN;
locPosSpan = this._locPosSpan = BROKEN;
} else {
locPosSpan = this.#locPosSpan = new HbsSpan(this.source, {
locPosSpan = this._locPosSpan = new HbsSpan(this.source, {
start,
end,
});
Expand Down Expand Up @@ -390,17 +390,17 @@ class CharPositionSpan implements SpanData {
export class HbsSpan implements SpanData {
readonly kind = OffsetKind.HbsPosition;

#charPosSpan: CharPositionSpan | BROKEN | null = null;
_charPosSpan: CharPositionSpan | BROKEN | null = null;

// the source location from Handlebars + AST Plugins -- could be wrong
#providedHbsLoc: SourceLocation | null;
_providedHbsLoc: SourceLocation | null;

constructor(
readonly source: Source,
readonly hbsPositions: { start: HbsPosition; end: HbsPosition },
providedHbsLoc: SourceLocation | null = null
) {
this.#providedHbsLoc = providedHbsLoc;
this._providedHbsLoc = providedHbsLoc;
}

serialize(): SerializedConcreteSourceSpan {
Expand All @@ -413,13 +413,13 @@ export class HbsSpan implements SpanData {
}

private updateProvided(pos: SourcePosition, edge: 'start' | 'end') {
if (this.#providedHbsLoc) {
this.#providedHbsLoc[edge] = pos;
if (this._providedHbsLoc) {
this._providedHbsLoc[edge] = pos;
}

// invalidate computed character offsets
this.#charPosSpan = null;
this.#providedHbsLoc = {
this._charPosSpan = null;
this._providedHbsLoc = {
start: pos,
end: pos,
};
Expand Down Expand Up @@ -466,19 +466,19 @@ export class HbsSpan implements SpanData {
}

toCharPosSpan(): CharPositionSpan | null {
let charPosSpan = this.#charPosSpan;
let charPosSpan = this._charPosSpan;

if (charPosSpan === null) {
let start = this.hbsPositions.start.toCharPos();
let end = this.hbsPositions.end.toCharPos();

if (start && end) {
charPosSpan = this.#charPosSpan = new CharPositionSpan(this.source, {
charPosSpan = this._charPosSpan = new CharPositionSpan(this.source, {
start,
end,
});
} else {
charPosSpan = this.#charPosSpan = BROKEN;
charPosSpan = this._charPosSpan = BROKEN;
return null;
}
}
Expand Down
12 changes: 6 additions & 6 deletions packages/@glimmer/syntax/lib/source/span-list.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,22 @@ export class SpanList {
return new SpanList(span.map(loc)).getRangeOffset(fallback);
}

#span: SourceSpan[];
_span: SourceSpan[];

constructor(span: SourceSpan[] = []) {
this.#span = span;
this._span = span;
}

add(offset: SourceSpan): void {
this.#span.push(offset);
this._span.push(offset);
}

getRangeOffset(fallback: SourceSpan): SourceSpan {
if (this.#span.length === 0) {
if (this._span.length === 0) {
return fallback;
} else {
let first = this.#span[0];
let last = this.#span[this.#span.length - 1];
let first = this._span[0];
let last = this._span[this._span.length - 1];

return first.extend(last);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/@glimmer/syntax/lib/symbol-table.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ export class ProgramSymbolTable extends SymbolTable {
private blocks = dict<number>();
private usedTemplateLocals: string[] = [];

#hasEval = false;
_hasEval = false;

getUsedTemplateLocals(): string[] {
return this.usedTemplateLocals;
}

setHasEval(): void {
this.#hasEval = true;
this._hasEval = true;
}

get hasEval(): boolean {
return this.#hasEval;
return this._hasEval;
}

has(name: string): boolean {
Expand Down

0 comments on commit c1ec710

Please sign in to comment.