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

Simplify scanning with skippable tokens #2006

Closed
wants to merge 1 commit into from
Closed

Simplify scanning with skippable tokens #2006

wants to merge 1 commit into from

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Jan 7, 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?

SkippableTokenAwareRuleWalker is totally overkill to just find the start and end position of identifiers, regex literals and template expressions.
I added the utility function getSkippableTokens to do exactly the same without the walker overhead. This function also takes advantage of proper tail calls if implemented in the vm.

Also the code for skipping tokens while scanning was copied to 5 different classes. I added a new function scanAllTokensWithSkip (name is for discussion) to address 4 of them.

Performance

I turns out that the overall performance gain for the 4 walkers is around 50%.
I profiled linting of tslint sources. Total time was 14s. Before this change each of EnableDisableRuleWalker, CommentFormatRule, JsdocFormatRule and NoTrailingWhitespaceRule accounted for around 260ms (~1.85% of total).
With this change each execution time reduces to under 120ms (~0.85% of total).

Is there anything you'd like reviewers to focus on?

whitespaceRule actually needs the walker to do its thing, so I left it untouched. Using utils.getSkippableTokens would walk the AST a second time. The slowdown is almost not measurable, but there is no real benefit either. Also the skipping logic here is different, as it needs to add failures before skipping.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

sweet perf numbers

@@ -73,6 +73,19 @@ export function scanAllTokens(scanner: ts.Scanner, callback: (s: ts.Scanner) =>
}
}

export function scanAllTokensWithSkip(scanner: ts.Scanner, skipMap: Map<number, number>, callback: (s: ts.Scanner) => void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add JSDoc comments to these two new util functions explaining why they are useful and in which cases they are better than the SkippableTokenAwareRuleWalker?

@ajafff
Copy link
Contributor Author

ajafff commented Jan 12, 2017

I'll close this PR as it doesn't seem worthwhile to have two separate implementations for the same task. This PR would only make sense if we could actually replace / deprecate SkippableTokenAwareRuleWalker with this.

While looking at #2009 I realized that this whole concept is going in the wrong direction:
We're trying to scan the grammar ourselves and try to skip all ranges where we don't know how to correctly handle tokens - that is:

  • identifier vs keyword
  • regex literal vs /, /=, etc.
  • template expressions
  • jsxText vs regular source code (e.g. comments, see comment-format TSX false positive #2007)
  • > > = vs >>= and even more cases that are currently not handled.

The more ranges we skip, the more tokens are not accessible for the rules to check. WhitespaceRule for example should check in template expressions. Or we need exceptions per rule like #2009 does for NoTrailingWhitespaceRule which could lead to other unexpected behaviour.

Why not hand the parsing off to the parser instead of trying hard to build something similar?

Right now I'm working on a function to visit every token without scanning. Hopefully with similar performance.
This could really replace SkippableTokenAwareRuleWalker and scanAllTokens. Should be ready this week.

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.

None yet

2 participants