Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf: use regexp in string escaping #553

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

ivan-tymoshenko
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko commented Oct 15, 2022

Do you want some perf upgrades? I've got something for you.

@ivan-tymoshenko
Copy link
Member Author

Снимок экрана 2022-10-15 в 15 22 11

Снимок экрана 2022-10-15 в 15 22 24

@ivan-tymoshenko ivan-tymoshenko force-pushed the use-regexp-for-string-escaping branch from c6408cf to c683091 Compare October 15, 2022 12:25
@@ -94,6 +97,10 @@ module.exports = class Serializer {
str = str.toString()
}

if (!STR_ESCAPE.test(str)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please include a comment above this line telling why it's needed?

@@ -94,6 +97,11 @@ module.exports = class Serializer {
str = str.toString()
}

// Fast escape chars check
if (!STR_ESCAPE.test(str)) {
Copy link
Member

Choose a reason for hiding this comment

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

This new check does not execute the str.length < 42 optimization. Is this wanted or could we go even faster?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mcollina mcollina merged commit 357c1e3 into master Oct 27, 2022
@mcollina mcollina deleted the use-regexp-for-string-escaping branch October 27, 2022 07:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants