-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
policy: minor perf opts and cleanup #29322
Conversation
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.
Multiple test-policy-* failures on all platforms in CI. Marking with "Request changes" so no one accidentally lands it until those are fixed. Feel free to dismiss this request-for-changes once the tests are passing.
@@ -34,10 +34,10 @@ const parse = (str) => { | |||
const entries = []; | |||
while (match = RegExpPrototype.exec(kSRIPattern, str)) { | |||
if (match.index !== prevIndex) { |
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.
Could it be one whole condition with below ?
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.
could be yea, i think readability is better as is but if requested to change I could
bcf133b
to
ac3298a
Compare
PR-URL: #29322 Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 6ce87c0 |
PR-URL: #29322 Reviewed-By: James M Snell <jasnell@gmail.com>
Cleanup: Use more Maps instead of object dictionaries, use private fields instead of WeakMaps, minor error message cleanup.
Perf: Makes redirects lazier for parsing which was proving to be a large amount of work for relative URL strings and adds a parse cache.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes