Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Speedup parser (~50%) by using 'slice' and 'charCodeAt' directly #1756

Merged
merged 1 commit into from Feb 28, 2019
Merged

Speedup parser (~50%) by using 'slice' and 'charCodeAt' directly #1756

merged 1 commit into from Feb 28, 2019

Conversation

IvanGoncharov
Copy link
Member

image

mjmahone
mjmahone approved these changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

Odd that a single function indirection would cause such a large performance improvement. Thanks for finding and making this change!

IvanGoncharov
IvanGoncharov commented Feb 28, 2019
View reviewed changes
src/language/lexer.js
@@ -137,9 +137,6 @@ export function getTokenDesc(token: Token): string {
return value ? `${token.kind} "${value}"` : token.kind;
}

const charCodeAt = String.prototype.charCodeAt;
const slice = String.prototype.slice;

Copy link
Member Author

Choose a reason for hiding this comment

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

This code was added in the initial commit(b5ed31e) so I missing context why it was added in the first place and is it safe to remove.
Even thought parsing is not the most critical function 50% speed up is very big.

@leebyron @mjmahone @Neitsch Can you please review and comment on whenever it is safe to use slice and charCodeAt directly?

@IvanGoncharov IvanGoncharov merged commit 8013c0d into graphql:master Feb 28, 2019
@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Mar 10, 2019
@snyk-bot snyk-bot mentioned this pull request Aug 31, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjmahone mjmahone mjmahone approved these changes

@leebyron leebyron Awaiting requested review from leebyron

@Neitsch Neitsch Awaiting requested review from Neitsch

Assignees
No one assigned
Labels
CLA Signed PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@IvanGoncharov @mjmahone @facebook-github-bot