Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Performance improvements pt. 1 #1977

Closed
wants to merge 4 commits into from
Closed

Performance improvements pt. 1 #1977

wants to merge 4 commits into from

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Jan 3, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update
  • Enable CircleCI for your fork (https://circleci.com/add-projects)

What changes did you make?

This is a first attempt to improve performance of core rules. It's basically #1747 ... search for \.get((Full)?Text|Start|(LeadingTrivia)?Width|(First|Last)Token|Child(ren|Count))\(\)|\.getChildAt\(\d+\) and pass SourceFile as parameter.

To keep the diff somewhat small, I chose to submit the changes in chunks as I progress. So here we go from alignRule until noDefaultExportRule.

Of course this is only the low hanging fruit. When I find the time, I will look at the rules in more detail.

@ajafff
Copy link
Contributor Author

ajafff commented Jan 3, 2017

As a side note: Maybe we could create a tslint rule for that? Maybe in a separate project as this would not be relevant for end users of tslint?

return modifiers.some((m) => {
return modifierKinds.some((k) => m.kind === k);
});
for (const modifier of modifiers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this changed?

const minEnd = Math.min(interval.endPosition, failure.getEndPosition().getPosition());
return maxStart <= minEnd;
});
const failureStart = failure.getStartPosition().getPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

why change?

/**
* Finds a child of a given node with a given kind.
* Note: This uses `node.getChildren()`, which does extra parsing work to include tokens.
*/
export function childOfKind(node: ts.Node, kind: ts.SyntaxKind): ts.Node | undefined {
return find(node.getChildren(), (child) => child.kind === kind);
export function childOfKind(node: ts.Node, kind: ts.SyntaxKind, sourceFile?: ts.SourceFile): ts.Node | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

why change?

@nchen63
Copy link
Contributor

nchen63 commented Jan 3, 2017

I'm wary of making perf improvements without a performance profiler indicating a problem or a benchmark improvement.

@ericanderson
Copy link
Member

I can't speak to whether or not there is a performance issue that needs to be fixed, but for loops seem to nearly always be faster than method invocations.

A simple test: http://jsperf.com/array-some-vs-loop

@ajafff
Copy link
Contributor Author

ajafff commented Jan 4, 2017

@ericanderson you're right at the moment, but eventually there will be no difference, hopefully.

However, that specific commit was not intended to be included in this or any other PR, so I reverted it.

@ajafff
Copy link
Contributor Author

ajafff commented Jan 5, 2017

@nchen63 You're right, I should have done the profiling before changing everything.
I just profiled some pretty big libraries including TypeScript with only one-line-Rule enabled, which has the most usages of node.getChildren and friends. The result was crushing this PR:
Out of 24s runtime only 16ms were spent in getSourceFileOfNode.

I'll close this PR and maybe you can even close #1747.

@nchen63
Copy link
Contributor

nchen63 commented Jan 5, 2017

@ajafff thanks for looking into that

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.

3 participants