http: explain the unused argument in IncomingMessage._read#37275
http: explain the unused argument in IncomingMessage._read#37275Ayase-252 wants to merge 4 commits intonodejs:masterfrom
Conversation
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
LGTM!
Note to whoever lands this: there's a typo in the commit message that needs fixing (ImcomingMessage -> IncomingMessage).
|
cc @nodejs/http |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
This is needed for performance reason. This adds an additional frame in a very hot code path.
See also: https://bugs.chromium.org/p/v8/issues/detail?id=10201
I'm -1 (strongly). This might be removed in the future.
|
@mcollina Thank you for giving the underlying performance consideration here. By giving that, this refactor should not be merged certainly. Should we add a comment here to explain the unused parameter? |
|
Yes please! |
Ayase-252
force-pushed
the
refactor/remove-unused-parameter
branch
from
00ec72b to
3bc6cfa
Compare
Ayase-252
changed the title
|
FWIW this might not be a problem anymore after v8 8.9 https://v8.dev/blog/v8-release-89 |
|
@ronag It' awesome. I added a NOTE comment as remind of the posiblity of improvement when v8 engine is upgraded to v8.9 in the future. |
|
The failures seem related to memory exhaustion during compilation, or nothing related to these changes anyway. I am going to land this. |
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Landed in ad3ebed...9d2125e |
|
@dnlup should the commits be squashed into a single commit before landing? The commits seem to be addressing the same task of throwing light on the unused argument. |
|
@RaisinTen I have seen landing both multiple commits and squashed commits in other PRs. In this case, I thought keeping the last two commits would have given a bit more context (perhaps making a mistake?) |
@RaisinTen I see. It makes sense. However, maybe squashing now would be too much hassle? (Reverting then landing again, I guess). Thanks for pointing that out, though. |
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #37275 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
8 participants
It removes parameter
nofIncomingMessagesince it is not used in the function body.