Skip to content

[WIP] Scanner Refactoring & Optimization #58728

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

Closed

Conversation

graphemecluster
Copy link
Contributor

@graphemecluster graphemecluster commented May 31, 2024

This involves only the scanner, and includes refactoring like preferring charCodeChecked and codePointChecked over bound checks plus charCodeUnchecked and codePointUnchecked, avoiding the use of string literals and string index property accessing, and preferring String#substring over String#slice due to performance.

Part of the above is already done in #58362.

Originally I would like to eliminate all regexes inside the scanner, but they don’t seem to affect performance at all, so I ended up abandoning the related changes.

This PR shouldn’t affect anything other than performance.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 31, 2024
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@@ -1127,7 +1116,7 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
* `pos` is outside the bounds set for `text`, `CharacterCodes.EOF` is returned instead.
*/
function codePointChecked(pos: number) {
return pos >= 0 && pos < end ? codePointUnchecked(pos) : CharacterCodes.EOF;
return pos < end ? codePointUnchecked(pos) : CharacterCodes.EOF;
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 carefully examined all expressions involving pos-- and pos -= number and determined that the pos >= 0 check is unnecessary.

@jakebailey
Copy link
Member

Is this the same as #58362?

@@ -2559,7 +2550,7 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
let regExpFlags = RegularExpressionFlags.None;
while (true) {
const ch = codePointChecked(pos);
if (ch === CharacterCodes.EOF || !isIdentifierPart(ch, languageVersion)) {
if (!isIdentifierPart(ch, languageVersion)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is unnecessary since isIdentifierPart(CharacterCodes.EOF, languageVersion) must return false (because CharacterCodes.EOF <= CharacterCodes.maxAsciiCharacter).

@@ -2997,7 +2988,7 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
}

function isClassContentExit(ch: number) {
return ch === CharacterCodes.closeBracket || ch === CharacterCodes.EOF || pos >= end;
return ch === CharacterCodes.closeBracket || ch === CharacterCodes.EOF;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unnecessary as all isClassContentExit calls are directly after charCodeChecked.

@graphemecluster graphemecluster marked this pull request as draft May 31, 2024 22:46
@graphemecluster
Copy link
Contributor Author

Is this the same as #58362?

Ah, I overlooked this. Let me omit all bound-checking related changes and leave the rest then.

@graphemecluster graphemecluster changed the title Perform More Bound Checks in Scanner with Refactoring & Optimization [WIP] Scanner Refactoring & Optimization May 31, 2024
@graphemecluster
Copy link
Contributor Author

I guess I need to get into the habit of searching for relevant PRs, not just issues, in a search engine, before open up one.

@graphemecluster
Copy link
Contributor Author

Fine, let me close this first since the branch cannot be renamed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants