-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
v8: fix RegExp nits in v8_prof_polyfill.js #13709
v8: fix RegExp nits in v8_prof_polyfill.js #13709
Conversation
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split().
cc @nodejs/v8, @nodejs/performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 6b9500b |
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Should this be backported to v6.x? |
It cherry-picks cleanly. I don't think it could cause any issue. |
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
I landed this on v6.x-staging and it seems to have been responsible for some failures on AIX + certain flavors of windows. I've backed it out for now and doing testing to see if the suite passes with this temporarily removed failure below
not ok 56 parallel/test-child-process-detached
---
duration_ms: 0.820
severity: fail
stack: |-
... edit: appears unrelated. going to bisect on machine and likely reland soon edit 2: relanded |
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
* Do not repeat RegExp creation in cycle. * Use sufficient string instead of RegExp in split(). PR-URL: #13709 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
v8
split()
.I am not sure about the status of this lib file (it is excluded from linting and is not tested in common CI). I've stumbled upon these fragments in some recent overall RegExp checks. The fixes seem rather simple and safe, but please suggest the ways we should test them if needed.