Skip to content
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

src: create extras internal binding #18420

Closed
wants to merge 3 commits into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jan 28, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 28, 2018
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere? If not I’d rather defer this PR until a solid use case materializes.

@devsnek
Copy link
Member Author

devsnek commented Jan 28, 2018

@TimothyGu i'd like to at least use createPrivateSymbol in place of our existing weakmap usage

@TimothyGu
Copy link
Member

@devsnek Are there existing places where WeakMap is used? If so, please change them in this PR. We don’t want currently unused and untested code lying around.

@devsnek
Copy link
Member Author

devsnek commented Jan 28, 2018

@TimothyGu not at the moment but incoming code for vm.Module uses it, this pr can be kept open until that lands i guess., i can also replace the uncurryThis in internal/util/types

@devsnek
Copy link
Member Author

devsnek commented Jan 28, 2018

@TimothyGu i made a slight change to the internal binding to better expose it, you might wanna take another look

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 29, 2018

The things in V8's prologue.js are internal helpers, they're not intended for public consumption. That doesn't mean you absolutely can't use them but there would have to be compelling reasons and a fallback plan for when they change or disappear.

By the way, lib/extras - probably means it can be require()'d by user code.

@devsnek
Copy link
Member Author

devsnek commented Jan 29, 2018

@bnoordhuis it was my understanding they were intended to be used by embedders, and it actually can't be required because it's not packed into node_javascript.cc

@bnoordhuis
Copy link
Member

The ability to add bindings, yes; not that particular bindings file, as far as I know.

@hashseed Are the functions in prologue.js intended to be used by embedders?

@TimothyGu
Copy link
Member

@hashseed
Copy link
Member

V8 extras has been an experiment with blink to implement some blink features in JavaScript. It has never really taken off, and we are now migrating most builtins in V8 away from JavaScript.

I would advise against exposing and later on relying on V8 extras bindings. It is somewhat debatable, but I wouldn't consider them official APIs.

If you want to create private symbols using V8's C++ API.

@TimothyGu
Copy link
Member

TimothyGu commented Jan 29, 2018

@hashseed

If you want to create private symbols using V8's C++ API.

v8::Private cannot be exposed to JavaScript without some extra nasty reinterpret_casting, so for every get/set operation one would have to go through C++. We had something like this (in fact we still use it, for some limited non-performance-intensive applications), but the performance cost is too great for us to do much with it.

It is somewhat debatable, but I wouldn't consider them official APIs.

@gsathya recommended using V8 Extras APIs for util.promisify: nodejs/promises#31 (comment). Has V8's official position on Extras changed since then?

If so it would be very sad, and would invalidate some creative applications of V8 Extras like implementation of a context-aware EventEmitter class.

@hashseed
Copy link
Member

@bmeurer

@bmeurer
Copy link
Member

bmeurer commented Jan 30, 2018

To be honest: V8 Extras is not well tested and a lot of it is legacy code now, especially the things added for performance reasons. I guess if we seriously want to use it outside of it's initial scope we could that, but then we should shrink it to the bare minimum and throw a lot of tests at it.

@devsnek
Copy link
Member Author

devsnek commented Jan 30, 2018

perhaps there could be like a Symbol::Private subclass that just flipped set_is_private, or maybe Symbol::New(isolate, some string, optional boolean is private)

@domenic
Copy link
Contributor

domenic commented Jan 30, 2018

FWIW my interpretation of V8 extras' scope has always included these kind of Node.js use cases. But, without a second consumer maintaining it has been tricky. Shrinking away some things (like, as you mentioned, simpleBind) would be good. I think it has a pretty good set of tests but more is always nice.

@devsnek
Copy link
Member Author

devsnek commented Jan 31, 2018

@domenic or @bmeurer or @hashseed do private symbols get optimized into the shape of a constructor?

as an example

const x = createPrivateSymbol('x');

function XWrap() {
  this.t = undefined; // i know this line matters
  this[x] = undefined; // does this line matter
}

XWrap.prototype.fillX = function(val) {
  this[x] = val;
}

@TimothyGu
Copy link
Member

@devsnek Yes. See the first part of https://v8project.blogspot.com/2018/01/hash-code.html, which indirectly talks about private symbols.

@devsnek
Copy link
Member Author

devsnek commented Jan 31, 2018

v8 team encouraged us to not use this, as they are trying to get rid of it, and to settle for weakmaps until private class fields land

@devsnek devsnek closed this Jan 31, 2018
@devsnek devsnek deleted the feature/v8-extras branch January 31, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants