-
Notifications
You must be signed in to change notification settings - Fork 513
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
parser improvements #680
parser improvements #680
Conversation
Codecov Report
@@ Coverage Diff @@
## main #680 +/- ##
==========================================
+ Coverage 97.25% 97.50% +0.24%
==========================================
Files 26 26
Lines 2004 2122 +118
==========================================
+ Hits 1949 2069 +120
+ Misses 55 53 -2
Continue to review full report at Codecov.
|
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.
the changes looks good but CI is failing
if (ret !== 0) { | ||
return ret | ||
} | ||
currentParser[kStatusMessage] = cstr(at, length) | ||
return 0 |
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.
if this function is empty, is it still needed?
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.
Yes, otherwise the WASM fails
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.
@dnlup we might want to make this optional there, it will save a few cycle.
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.
Yes, all the functions exported in https://github.com/nodejs/undici/blob/main/deps/llhttp/src/api.c#L29 must be provided in the initialization.
if (ret !== 0) { | ||
return ret | ||
} | ||
currentParser[kStatusMessage] = cstr(at, length) | ||
return 0 |
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.
Yes, all the functions exported in https://github.com/nodejs/undici/blob/main/deps/llhttp/src/api.c#L29 must be provided in the initialization.
lib/client.js
Outdated
@@ -816,7 +798,11 @@ function onSocketError (err) { | |||
} | |||
|
|||
function onSocketData (data) { | |||
this[kParser].execute(data) | |||
try { |
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.
I see a small drop in the benchmarks, I'll run them again to be sure the use of the try
/catch
is having an impact.
https://github.com/nodejs/undici/runs/2275815074?check_suite_focus=true
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.
Removing the try catch should be easy. I’ll fix it.
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.
On a second run, benchmarks look good. I would not change it yet.
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
Does it impact our benchmarks?
Benchmarks look good |
* refactor: parser upgrade * refactor: parser onBody * refactor: parser onmessagecomplete * perf: parser pre-allocate array * refactor: parser onHeaders * refactor: parser simplify * fix: remove unused var * fix: add assertion * refactor: inline makeError * refactor: parse this[kBufferSize] * refactor: parser don't are about statusMessage * fixup! fix: add assertion * fix * fix: parser kHeaders * fixup * fixup * fixuP * fixuP * fixuP * fixuP * fixuP * fixuP * fixuP * fixuP * fixuP * fixup: assertion * fixup * fixup * fixup * fixup * fixup: avoid try/catch
Refactors and simplifies parser.