http: explain the unused argument in IncomingMessage._read #37275
Conversation
|
cc @nodejs/http |
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]>
It removes parameter
nofIncomingMessagesince it is not used in the function body.