-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
req & res in http.createServer have not the same map #6294
Comments
From what I've seen, the _idlePrev and _idleNext properties on both req and res can have different maps. |
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L71 , This Line add dynamic property for IncomingMessage, It caused IncomingMessage instance have not the same map. |
I don't understand how this is a problem... Closing, let us know if we need to re-open. |
@Fishrock123 It means request and response objects are implemented much less efficiently than they can be. The issue is legitimate, I'll reopen (and try to look at it this week.) |
Request and Reponse object in createServer has not same map, So it can not use Hidden Classes optimizing in V8, Hidden Classes is about 2x faster when get class instance. https://developers.google.com/v8/design#fast-property-access |
I had a look and I filed #7003 as a basic sanity check. It doesn't catch this particular issue but I've come to believe the test case is something of a red herring because it's comparing objects at different phases in their life cycle. If you rewrite the test to only check at the end (like I did in #7003), everything is fine. That's not to say the http module is currently a paragon of efficiency - it's not - but this particular issue doesn't seem valid. I'll close it for now. |
Add a basic regression test that checks if the map for IncomingMessage and OutgoingMessage objects is stable over time. The test is not exhaustive in that it doesn't try to establish whether the transition path is the same on every request, it just checks that objects in their final states have the same map. To be investigated why the first (and only the first) ServerRequest object ends up with a deprecated map, regardless of the number of iterations. PR-URL: nodejs#7003 Refs: nodejs#6294 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add a basic regression test that checks if the map for IncomingMessage and OutgoingMessage objects is stable over time. The test is not exhaustive in that it doesn't try to establish whether the transition path is the same on every request, it just checks that objects in their final states have the same map. To be investigated why the first (and only the first) ServerRequest object ends up with a deprecated map, regardless of the number of iterations. PR-URL: #7003 Refs: #6294 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add a basic regression test that checks if the map for IncomingMessage and OutgoingMessage objects is stable over time. The test is not exhaustive in that it doesn't try to establish whether the transition path is the same on every request, it just checks that objects in their final states have the same map. To be investigated why the first (and only the first) ServerRequest object ends up with a deprecated map, regardless of the number of iterations. PR-URL: #7003 Refs: #6294 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add a basic regression test that checks if the map for IncomingMessage and OutgoingMessage objects is stable over time. The test is not exhaustive in that it doesn't try to establish whether the transition path is the same on every request, it just checks that objects in their final states have the same map. To be investigated why the first (and only the first) ServerRequest object ends up with a deprecated map, regardless of the number of iterations. PR-URL: #7003 Refs: #6294 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
use
node --allow-natives-syntax index.js
to test.The text was updated successfully, but these errors were encountered: