Skip to content

Transformation API Cleanup #11165

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

Merged
merged 7 commits into from
Sep 30, 2016
Merged

Transformation API Cleanup #11165

merged 7 commits into from
Sep 30, 2016

Conversation

rbuckton
Copy link
Contributor

This PR cleans up and improves the design of the transformer and pretty printer, and reduces the overall size of Node. Notable changes include:

  • Moved emitFlags, sourceMapRange, and commentRange to EmitNode object.
    • This reduces the overall size of Node and seems to reduce the total memory used by 5-8MB (based on the "Memory Used" statistic as part of our performance tests.
  • Moved getNodeEmitFlags, setNodeEmitFlags, etc. out of transformer.ts to factory.ts.
  • Cleaned up the emit pipeline in emitter.ts.
  • Moved emitNodeWithSourceMaps into sourcemaps.ts.
  • Moved emitNodeWithNotification into transformer.ts.
  • Moved trySubstituteNode into transformer.ts (as emitNodeWithSubstitution).
  • Moved tryEmitConstantValue into ts.ts as part of substitution.
  • Removed unused and deprecated functionality from sourcemap.ts
  • Moved chain and compose from transformer.ts to core.ts
  • Fixes an issue when emitting PropertyAccessExpression for a const enum value when comments are emitted.

In addition to the aforementioned reduction in memory footprint, this results in a small performance improvement in overall compiler time for most scenarios (between 0.6% to 1.5% improvement).

@rbuckton rbuckton changed the title Emit node Transformation API Cleanup Sep 27, 2016
*
* If the token's end position is synthetic (undefined or a negative value), no mapping
* will be created.
* Emits a token of a node node with possible leading and trailing source maps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Emits a token of a node ... instead of Emits a token of a node node ...

*
* NOTE: Do not call this method directly. It is part of the emit pipeline
* and should only be called indirectly from pipelineEmitWithSourceMap or
* pipelineEmitInUnspecifiedContext (when pickign a more specific context).
Copy link
Contributor

Choose a reason for hiding this comment

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

"picking"

@@ -2798,6 +2798,20 @@ namespace ts {
return tokenSourceMapRanges && tokenSourceMapRanges[token];
}

export function getConstantValue(node: Node) {
const emitNode = node.emitNode;
if (emitNode && emitNode.flags & EmitFlags.ConstantValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why can't you just check if there exists emitNode.constantValue instead of using the EmitFlags.ConstatntValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably doesn't matter. Since constantValue is an expando property, I was trying to avoid some deoptimization cases.

@@ -23,15 +23,15 @@ let c1 = Foo["C"].toString();


//// [constEnumToStringWithComments.js]
var x0 = 100 /* X */..toString();
var x1 = 100 /* "X" */..toString();
var x0 = 100 /* X */.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why one of the "." is gone? Also realize that space after number is important 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should only ever emit .. if the preceding text is an integer literal. We do not need to emit .. if the preceding text is a decimal literal, whitespace, a comment, or anything else. 100/**/..toString() is not legal JavaScript, so the previous outcome for this test was invalid.

@@ -5,7 +5,7 @@ namespace ts {
export interface CommentWriter {
reset(): void;
setSourceFile(sourceFile: SourceFile): void;
emitNodeWithComments(node: Node, emitCallback: (node: Node) => void): void;
emitNodeWithComments(emitContext: EmitContext, node: Node, emitCallback: (emitContext: EmitContext, node: Node) => void): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

at first it is a bit confusing with emitContext and context which is a TransformationContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to a better name, but I feel this is the correct terminology.

@rbuckton rbuckton merged commit edd8eb8 into master Sep 30, 2016
@rbuckton rbuckton deleted the emitNode branch September 30, 2016 00:44
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants