Skip to content

[Transforms] Performance fixes#8824

Merged
rbuckton merged 13 commits into
transformsfrom
transforms-fixPerformance
May 27, 2016
Merged

[Transforms] Performance fixes#8824
rbuckton merged 13 commits into
transformsfrom
transforms-fixPerformance

Conversation

@rbuckton
Copy link
Copy Markdown
Contributor

This PR addresses several performance issues in tree transformations. This includes:

  • A new internal performance API, which helps to avoid deoptimizations on the ts namespace in node.
  • A new (temporary) --extendedDiagnostics compiler flag which dumps all of the performance marks and measures.
  • Storage of transient transformation properties such as NodeEmitFlags and custom ranges for comments and source maps on the node.
  • Caching for getNodeEmitFlags to speed up repeated lookups.

@rbuckton rbuckton added the Domain: API: Transforms Relates to the public transform API label May 25, 2016
@rbuckton
Copy link
Copy Markdown
Contributor Author

CC: @vladima, @yuit, @mhegazy, @sandersn

@rbuckton
Copy link
Copy Markdown
Contributor Author

Ping

Comment thread src/compiler/factory.ts
// Create an identifier and give it a parent. This allows us to resolve the react
// namespace during emit.
const react = createIdentifier(reactNamespace || "React");
react.flags &= ~NodeFlags.Synthesized;
Copy link
Copy Markdown
Contributor

@yuit yuit May 26, 2016

Choose a reason for hiding this comment

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

should similar things be applied in createTempVariable ?
EDIT: I didnt' see "~" @rbuckton explains why he needs to unset the flag in here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this is a specific case to ensure the react namespace is properly resolved by the emit resolver, which would otherwise try to find an original source tree node that doesn't exist.

@yuit
Copy link
Copy Markdown
Contributor

yuit commented May 27, 2016

👍

* transformation. This ensures transient properties related to transformations can be safely
* stored on source tree nodes that may be reused across multiple transformations (such as
* with compile-on-save).
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you comment on the lifetime of this transformId? You mentioned monotonically increasing but is that in a global context, or per compilation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It sounds like global (you mentioned compile-on-save), but clarify this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, globally. I need to ensure that I can clean up transient properties from source tree nodes.

@rbuckton rbuckton merged commit 2ed9789 into transforms May 27, 2016
@rbuckton rbuckton deleted the transforms-fixPerformance branch May 31, 2016 20:21
@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

Domain: API: Transforms Relates to the public transform API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants