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.

Sign up for GitHub

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

Jump to bottom

refactor(compiler): produce more dense generated code #16666

Merged
merged 1 commit into from May 11, 2017
Merged

refactor(compiler): produce more dense generated code #16666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 40 additions & 25 deletions packages/compiler/src/output/abstract_emitter.ts
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ export abstract class OutputEmitter {
}

class _EmittedLine {
partsLength = 0;
parts: string[] = [];
srcSpans: (ParseSourceSpan|null)[] = [];
constructor(public indent: number) {}
Expand Down Expand Up @@ -51,9 +52,14 @@ export class EmitterVisitorContext {

lineIsEmpty(): boolean { return this._currentLine.parts.length === 0; }

lineLength(): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is checked in a loop as items are added to the list this results in an O(n^2) complexity to appending to a line. Consider using a tracking the line length as parts are added the _currentLine.parts array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return this._currentLine.indent * _INDENT_WITH.length + this._currentLine.partsLength;
}

print(from: {sourceSpan: ParseSourceSpan | null}|null, part: string, newLine: boolean = false) {
if (part.length > 0) {
this._currentLine.parts.push(part);
this._currentLine.partsLength += part.length;
this._currentLine.srcSpans.push(from && from.sourceSpan || null);
}
if (newLine) {
Expand All @@ -69,12 +75,16 @@ export class EmitterVisitorContext {

incIndent() {
this._indent++;
this._currentLine.indent = this._indent;
if (this.lineIsEmpty()) {
this._currentLine.indent = this._indent;
}
}

decIndent() {
this._indent--;
this._currentLine.indent = this._indent;
if (this.lineIsEmpty()) {
this._currentLine.indent = this._indent;
}
}

pushClass(clazz: o.ClassStmt) { this._classes.push(clazz); }
Expand Down Expand Up @@ -420,24 +430,18 @@ export abstract class AbstractEmitterVisitor implements o.StatementVisitor, o.Ex
return null;
}
visitLiteralArrayExpr(ast: o.LiteralArrayExpr, ctx: EmitterVisitorContext): any {
const useNewLine = ast.entries.length > 1;
ctx.print(ast, `[`, useNewLine);
ctx.incIndent();
this.visitAllExpressions(ast.entries, ctx, ',', useNewLine);
ctx.decIndent();
ctx.print(ast, `]`, useNewLine);
ctx.print(ast, `[`);
this.visitAllExpressions(ast.entries, ctx, ',');
ctx.print(ast, `]`);
return null;
}
visitLiteralMapExpr(ast: o.LiteralMapExpr, ctx: EmitterVisitorContext): any {
const useNewLine = ast.entries.length > 1;
ctx.print(ast, `{`, useNewLine);
ctx.incIndent();
ctx.print(ast, `{`);
this.visitAllObjects(entry => {
ctx.print(ast, `${escapeIdentifier(entry.key, this._escapeDollarInStrings, entry.quoted)}: `);
ctx.print(ast, `${escapeIdentifier(entry.key, this._escapeDollarInStrings, entry.quoted)}:`);
entry.value.visitExpression(this, ctx);
}, ast.entries, ctx, ',', useNewLine);
ctx.decIndent();
ctx.print(ast, `}`, useNewLine);
}, ast.entries, ctx, ',');
ctx.print(ast, `}`);
return null;
}
visitCommaExpr(ast: o.CommaExpr, ctx: EmitterVisitorContext): any {
Expand All @@ -446,24 +450,35 @@ export abstract class AbstractEmitterVisitor implements o.StatementVisitor, o.Ex
ctx.print(ast, ')');
return null;
}
visitAllExpressions(
expressions: o.Expression[], ctx: EmitterVisitorContext, separator: string,
newLine: boolean = false): void {
this.visitAllObjects(
expr => expr.visitExpression(this, ctx), expressions, ctx, separator, newLine);
visitAllExpressions(expressions: o.Expression[], ctx: EmitterVisitorContext, separator: string):
void {
this.visitAllObjects(expr => expr.visitExpression(this, ctx), expressions, ctx, separator);
}

visitAllObjects<T>(
handler: (t: T) => void, expressions: T[], ctx: EmitterVisitorContext, separator: string,
newLine: boolean = false): void {
handler: (t: T) => void, expressions: T[], ctx: EmitterVisitorContext,
separator: string): void {
let incrementedIndent = false;
for (let i = 0; i < expressions.length; i++) {
if (i > 0) {
ctx.print(null, separator, newLine);
if (ctx.lineLength() > 80) {
ctx.print(null, separator, true);
if (!incrementedIndent) {
// continuation are marked with double indent.
ctx.incIndent();
ctx.incIndent();
incrementedIndent = true;
}
} else {
ctx.print(null, separator, false);
}
}
handler(expressions[i]);
}
if (newLine) {
ctx.println();
if (incrementedIndent) {
// continuation are marked with double indent.
ctx.decIndent();
ctx.decIndent();
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/output/js_emitter.ts
View file
Open in desktop
Expand Up @@ -62,7 +62,7 @@ class JsEmitterVisitor extends AbstractJsEmitterVisitor {
if (filePath != this._genFilePath) {
let prefix = this.importsWithPrefixes.get(filePath);
if (prefix == null) {
prefix = `import${this.importsWithPrefixes.size}`;
prefix = `i${this.importsWithPrefixes.size}`;
this.importsWithPrefixes.set(filePath, prefix);
}
ctx.print(ast, `${prefix}.`);
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/output/ts_emitter.ts
View file
Open in desktop
Expand Up @@ -392,7 +392,7 @@ class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor
if (filePath != this._genFilePath) {
let prefix = this.importsWithPrefixes.get(filePath);
if (prefix == null) {
prefix = `import${this.importsWithPrefixes.size}`;
prefix = `i${this.importsWithPrefixes.size}`;
this.importsWithPrefixes.set(filePath, prefix);
}
ctx.print(null, `${prefix}.`);
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/test/aot/compiler_spec.ts
View file
Open in desktop
Expand Up @@ -386,7 +386,7 @@ describe('compiler (unbundled Angular)', () => {
const mainNgFactory = genFiles.find(gf => gf.srcFileUrl === '/app/main.ts');
const flags = NodeFlags.TypeDirective | NodeFlags.Component | NodeFlags.OnDestroy;
expect(mainNgFactory.source)
.toContain(`${flags},(null as any),0,import1.Extends,[import2.AParam]`);
.toContain(`${flags},(null as any),0,i1.Extends,[i2.AParam]`);
});
}));

Expand Down Expand Up @@ -438,7 +438,7 @@ describe('compiler (unbundled Angular)', () => {
const mainNgFactory = genFiles.find(gf => gf.srcFileUrl === '/app/main.ts');
const flags = NodeFlags.TypeDirective | NodeFlags.Component | NodeFlags.OnDestroy;
expect(mainNgFactory.source)
.toContain(`${flags},(null as any),0,import1.Extends,[import2.AParam_2]`);
.toContain(`${flags},(null as any),0,i1.Extends,[i2.AParam_2]`);
});
}));

Expand Down
44 changes: 21 additions & 23 deletions packages/compiler/test/aot/jit_summaries_spec.ts
View file
Open in desktop
Expand Up @@ -44,11 +44,11 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`);
expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`);
expect(genFile.source).toContain('export function MyServiceNgSummary()');
// Note: CompileSummaryKind.Injectable = 3
expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import0.MyService/);
expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}');
expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i0.MyService/);
expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}');
}));

it('should create @Pipe summaries', fakeAsync(() => {
Expand All @@ -72,11 +72,11 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`);
expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`);
expect(genFile.source).toContain('export function MyPipeNgSummary()');
// Note: CompileSummaryKind.Pipe = 1
expect(genFile.source).toMatch(/summaryKind: 0,\s*type: \{\s*reference: import0.MyPipe/);
expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}');
expect(genFile.source).toMatch(/summaryKind:0,\s*type:\{\s*reference:i0.MyPipe/);
expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}');
}));

it('should create @Directive summaries', fakeAsync(() => {
Expand All @@ -100,12 +100,11 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`);
expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`);
expect(genFile.source).toContain('export function MyDirectiveNgSummary()');
// Note: CompileSummaryKind.Directive = 1
expect(genFile.source)
.toMatch(/summaryKind: 1,\s*type: \{\s*reference: import0.MyDirective/);
expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}');
expect(genFile.source).toMatch(/summaryKind:1,\s*type:\{\s*reference:i0.MyDirective/);
expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}');
}));

it('should create @NgModule summaries', fakeAsync(() => {
Expand All @@ -126,11 +125,11 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`);
expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`);
expect(genFile.source).toContain('export function MyModuleNgSummary()');
// Note: CompileSummaryKind.NgModule = 2
expect(genFile.source).toMatch(/summaryKind: 2,\s*type: \{\s*reference: import0.MyModule/);
expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}');
expect(genFile.source).toMatch(/summaryKind:2,\s*type:\{\s*reference:i0.MyModule/);
expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}');
}));

it('should embed useClass provider summaries in @Directive summaries', fakeAsync(() => {
Expand Down Expand Up @@ -164,10 +163,10 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toMatch(/useClass: \{\s*reference: import1.MyService/);
expect(genFile.source).toMatch(/useClass:\{\s*reference:i1.MyService/);
// Note: CompileSummaryKind.Injectable = 3
expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import1.MyService/);
expect(genFile.source).toContain('token: {identifier: {reference: import1.Dep}}');
expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i1.MyService/);
expect(genFile.source).toContain('token:{identifier:{reference:i1.Dep}}');
}));

it('should embed useClass provider summaries into @NgModule summaries', fakeAsync(() => {
Expand Down Expand Up @@ -197,10 +196,10 @@ describe('aot summaries for jit', () => {
const genFile =
compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts');

expect(genFile.source).toMatch(/useClass: \{\s*reference: import1.MyService/);
expect(genFile.source).toMatch(/useClass:\{\s*reference:i1.MyService/);
// Note: CompileSummaryKind.Injectable = 3
expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import1.MyService/);
expect(genFile.source).toContain('token: {identifier: {reference: import1.Dep}}');
expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i1.MyService/);
expect(genFile.source).toContain('token:{identifier:{reference:i1.Dep}}');
}));

it('should reference declared @Directive and @Pipe summaries in @NgModule summaries',
Expand Down Expand Up @@ -327,13 +326,12 @@ describe('aot summaries for jit', () => {
lib3Gen.find(f => f.genFileUrl === '/lib3/reexport.ngsummary.ts');

// ngsummary.ts files should use the reexported values from direct and deep deps
expect(lib3ModuleNgSummary.source).toContain(`import * as i4 from '/lib2/module.ngsummary'`);
expect(lib3ModuleNgSummary.source)
.toContain(`import * as import4 from '/lib2/module.ngsummary'`);
expect(lib3ModuleNgSummary.source)
.toContain(`import * as import5 from '/lib2/reexport.ngsummary'`);
.toContain(`import * as i5 from '/lib2/reexport.ngsummary'`);
expect(lib3ModuleNgSummary.source)
.toMatch(
/export function Lib3ModuleNgSummary()[^;]*,\s*import4.Lib1Module_1NgSummary,\s*import4.Lib2ModuleNgSummary,\s*import5.ReexportModule_2NgSummary\s*\]\s*;/);
/export function Lib3ModuleNgSummary()[^;]*,\s*i4.Lib1Module_1NgSummary,\s*i4.Lib2ModuleNgSummary,\s*i5.ReexportModule_2NgSummary\s*\]\s*;/);

// ngsummaries should add reexports for imported NgModules from a deep dependency
expect(lib3ModuleNgSummary.source)
Expand Down
22 changes: 17 additions & 5 deletions packages/compiler/test/output/js_emitter_spec.ts
View file
Open in desktop
Expand Up @@ -115,7 +115,19 @@ export function main() {
expect(emitStmt(o.literal(true).toStmt())).toEqual('true;');
expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`'someStr';`);
expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`);
expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey: 1};`);
expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey:1};`);
});

it('should break expressions into multiple lines if they are too long', () => {
const values: o.Expression[] = new Array(100);
values.fill(o.literal(1));
values.splice(50, 0, o.fn([], [new o.ReturnStatement(o.literal(1))]));
expect(emitStmt(o.variable('fn').callFn(values).toStmt())).toEqual([
'fn(1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,',
' 1,1,1,1,1,1,1,1,1,1,function() {', ' return 1;',
' },1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,',
' 1,1,1,1,1,1,1,1,1,1,1,1);'
].join('\n'));
});

it('should support blank literals', () => {
Expand All @@ -126,19 +138,19 @@ export function main() {
it('should support external identifiers', () => {
expect(emitStmt(o.importExpr(sameModuleIdentifier).toStmt())).toEqual('someLocalId;');
expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([
`var import0 = re` +
`var i0 = re` +
`quire('somePackage/someOtherPath');`,
`import0.someExternalId;`
`i0.someExternalId;`
].join('\n'));
});

it('should support `importAs` for external identifiers', () => {
spyOn(importResolver, 'getImportAs')
.and.returnValue(new StaticSymbol('somePackage/importAsModule', 'importAsName', []));
expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([
`var import0 = re` +
`var i0 = re` +
`quire('somePackage/importAsModule');`,
`import0.importAsName;`
`i0.importAsName;`
].join('\n'));
});

Expand Down