search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108.02k stars 29.81k forks source link

Stop using V8::External #13503

Closed DemiMarie closed 6 years ago

DemiMarie commented 7 years ago

Node.js uses v8::External in many places. However, these are not efficiently implemented in v8: each of them consists of a full object that contains a pointer. Instead, Node should ensure that each of the pointers is aligned, and use GetAlignedPointerInInternalField/SetAlignedPointerInInternalField instead.

addaleax commented 7 years ago

Mh, looking through the source …

I wouldn’t expect that anything here has a significant negative performance impact.

DemiMarie commented 7 years ago

Would it be possible to instead use a custom FunctionTemplate instead? That would let Node typecheck it, which would avoid problems where bad JS code can cause Node to crash.

On Tue, Jun 6, 2017, 3:34 PM Anna Henningsen notifications@github.com wrote:

Mh, looking through the source …

  • env.h & env-inl.h: This should be fine, it’s just creating the object once and there’s not much we can do about these, I think. At least it’s not trivially replaced by using internal fields.
  • inspector_agent.cc: Maybe? This isn’t hot code, and I don’t know off the top of my head what kind of object the inspector object is.
  • node_api.cc: Yes, quite a few of those could probably be pointers. We’d just need to make sure that we keep in mind that pointers provided from userland can’t be assumed to be aligned.
  • node_contextify.cc: The object on which we operate can be passed from userland and doesn’t have any internal fields that would be available for use by us.
  • node_crypto.cc: These can’t be replaced by internal fields either, but I think the methods using them might be dead code (i.e. not used outside of our own tests).
  • node_http_parser.cc, tls_wrap.cc & stream_base-inl.h: Externals are used for passing around C++ objects in JS code; we might be able to get around this by some refactoring.

I wouldn’t expect that anything here has a significant negative performance impact.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/13503#issuecomment-306593192, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGWB1Xs1TeGSIa2oOOMVxhEIiR2b7fdks5sBam1gaJpZM4NxsKL .

bnoordhuis commented 7 years ago

I think you mean ObjectTemplate? Instantiating one with NewInstance() isn't much cheaper than External::New(). If anything, it's probably a little slower because it does more.

DemiMarie commented 7 years ago

I meant FunctionTemplate – V8 doesn’t (to my knowledge) allow one to typecheck ObjectTemplate.

On Jun 7, 2017 4:45 PM, "Ben Noordhuis" notifications@github.com wrote:

I think you mean ObjectTemplate? Instantiating one with NewInstance() isn't much cheaper than External::New(). If anything, it's probably a little slower because it does more.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/13503#issuecomment-306919152, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGWBw00qhLtTERQBYnbV3_sj1davOMvks5sBwvxgaJpZM4NxsKL .

bnoordhuis commented 7 years ago

FunctionTemplate doesn't have methods for defining extra slots but I infer you are referring to FunctionTemplate::InstanceTemplate(), which is an ObjectTemplate that controls the shape of instances.

The point stands though that it's probably going to be at least as expensive as an External. Yes, you can do HasInstance checks on instances but that seems like a fairly marginal improvement over Value::IsExternal(). It's not completely without value but neither is it worth spending a lot of effort on.

DemiMarie commented 7 years ago

The main advantage is to avoid segfault “bugs” by ensuring that bad JS cannot cause a crash in C++.

jasnell commented 7 years ago

Just as a point of reference, I should note that I'm currently using V8::External in a couple of ways in the http2 implementation. Specifically:

DemiMarie commented 7 years ago

Would it be possible to change those to use proper FunctionTemplates or to use symbol-keyed properties?

On Fri, Jun 16, 2017, 9:33 AM James M Snell notifications@github.com wrote:

Just as a point of reference, I should note that I'm currently using V8::External in a couple of ways in the http2 implementation. Specifically:

-

The StreamBase reference on a socket is currently stored in an External via the _external property. This reference is passed in to the http2 impl's native side so that the native node::Http2Session can read data from, and write data to, the StreamBase directly, without bouncing back out to the JS layer each time. That could be refactored, of course, but currently the _external is the only mechanism I have for getting the appropriate pointer.

When headers are received, I am wrapping inbound header field names in an external string to avoid unnecessary memcpy'ing. The header data is passed from the nghttp2 library in a persistent buffer that is released when the thing is garbage collected. If I was forced to treat header field values as UTF-8 for backwards compatibility reasons, I would be doing the same with those as it yields a significant performance improvement.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/13503#issuecomment-309027273, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGWByuy-iX0vNLVHh-2uc3jGFHHESUDks5sEoQ-gaJpZM4NxsKL .

jasnell commented 7 years ago

For the header names, not without adding a memcpy for every header field, which gets expensive very quickly.

TimothyGu commented 7 years ago

I'm working on getting rid of Externals in inspector, should be able to put up a PR by tomorrow.

bnoordhuis commented 6 years ago

This issue has languished and it doesn't look like there is consensus on the suggested changes so I'll go ahead and close it out.