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

n-api: cache Symbol.hasInstance #12246

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

commented Apr 5, 2017

This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

Re nodejs/abi-stable-node#209
Re nodejs/abi-stable-node#214

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

N-API

n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

Re nodejs/abi-stable-node#209
Re nodejs/abi-stable-node#214
@jasnell
jasnell approved these changes Apr 5, 2017

@mscdex mscdex added the n-api label Apr 5, 2017

}
v8::Isolate* isolate;
v8::Persistent<v8::Value> last_exception;
v8::Persistent<v8::Value> has_instance;

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

do we need a copy in every env ?

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 6, 2017

Member

ie could we share one copy across all envs ?

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Apr 7, 2017

Author Contributor

That brings back the question of what prompts the creation of a new env? Currently it is the loading of a module. But should it be a new isolate?

Also, how likely is this to be a large waste of memory? I mean, are we going to have tons of native modules loaded all at once?

I guess when there really start to appear multiple isolates (or whatever keys meaning "this JS world and not the other" because "this v8::Local<v8::Value> is only valid in this world") then we can re-examine our choice of one env per module, because the problem will have been solved upstream and we need only follow suit. We might then introduce a std::map keyed to whatever the engine then provides, or we may have a better mechanism for tacking data onto such a key and we'll once again cast.

... and we can do all this without breaking any ABI, since it's purely within our implementation.

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 7, 2017

Member

I just wondered if the exact same code, except that having has_instance be static would work. The cache would be created the first time an env was created. The only concern was whether this could happen in parallel, but given Node.js generally single thread nature that may not be an issue.

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Apr 7, 2017

Author Contributor

@mhdawson because eventually the env will be keyed to some kind of context, and we are preparing ourselves for that. Otherwise last_exception could also be static - and it used to be, and the env was precisely equal to the isolate. The problem is we can't currently tack onto the isolate because there is no reliable mechanism for that, so we're instantiating an environment per module.

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Apr 7, 2017

Author Contributor

So, yes, it would work perfectly, and it's only our desire to be proactive that prompted us to implement this solution.

This comment has been minimized.

Copy link
@mhdawson

mhdawson Apr 7, 2017

Member

ok, got it.

@TimothyGu

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Alternatively, we can convince V8 to add a v8::Symbol::GetHasInstance(v8::Isolate*) like the ones they already have for some other symbols.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

@TimothyGu why not go for the whole nine yards and ask them to expose the instanceof operator itself?

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Discussion of getting changes into v8 is a good follow on, but don't think it should block landing this PR.

CI run: https://ci.nodejs.org/job/node-test-pull-request/7270/

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

CI run was green landing.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

landed as 8fbace1

@mhdawson mhdawson closed this Apr 7, 2017

mhdawson added a commit that referenced this pull request Apr 7, 2017
n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

PR-URL: #12246
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@TimothyGu

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

@gabrielschulhof

why not go for the whole nine yards and ask them to expose the instanceof operator itself?

Indeed it's a much better idea. @hashseed @fhinkel, is Function::HasInstance(Local<Value>) something you would be interested in implementing?

@TimothyGu TimothyGu referenced this pull request Apr 8, 2017
0 of 3 tasks complete
@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2017

@gabrielschulhof @mhdawson

It appears this PR may be making a n-api test fail on Windows. Here is a CI run of current master: https://ci.nodejs.org/job/node-test-binary-windows/7558/

not ok 332 addons-napi/test_instanceof/test
  ---
  duration_ms: 0.193
  severity: fail
  stack: |-
    
    assert.js:82
      throw new assert.AssertionError({
      ^
    AssertionError: true === undefined
        at assertTrue (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2016\test\addons-napi\test_instanceof\test.js:16:17)
        at eval (eval at testFile (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2016\test\addons-napi\test_instanceof\test.js:38:3), <anonymous>:29:1)
        at testFile (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2016\test\addons-napi\test_instanceof\test.js:38:3)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2016\test\addons-napi\test_instanceof\test.js:42:1)
        at Module._compile (module.js:607:30)
        at Object.Module._extensions..js (module.js:618:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Module.runMain (module.js:643:10)

Perhaps something was changed since the last CI run? All other platforms seem to be unaffected.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

Perhaps something was changed since the last CI run?

@mscdex Looking for an n-api change that might have landed after CI was run for this one, the only candidate I'm seeing is 8460284. Maybe that change in conjunction with this one is the problem on Windows but either change by itself is fine? (Totally speculating here, but that's my best guess?)

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:209-cache-has-instance branch Apr 8, 2017

refack added a commit to refack/node that referenced this pull request Apr 8, 2017
@refack

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

solved regression by @gabrielschulhof in #12283

@hashseed

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

I'm considering adding v8::Function::HasInstance to the V8 API. What is napi_instanceof supposed to do? Is it exposing exactly what the instanceof operator does, including overriding behavior through @@hasInstance?

Is this what you need?

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Is it exposing exactly what the instanceof operator does, including overriding behavior through @@hasInstance?

Yes, the options were either doing that or calling it something other than napi_instanceof. ;)

Is this what you need?

Yes, looks like it. Thank you! 💚

@hashseed

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Alright. Will have to add some tests. It will not land before V8 6.0 though because we are cutting the 5.9 branch very soon.

@hashseed

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

I changed the API to v8::Value::InstanceOf so that the rhs can also be a non-v8::Function.

@hashseed

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

The changes for v8::Value::InstanceOf and v8::Symbol::GetHasInstance landed in V8 6.0.

Do we want to open an issue to track so that this can be used as soon as Node.js upgrades to V8 6.0?

@fhinkel

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

@hashseed Usually we have an issue for anything related to a V8 upgrade. Might make sense to start that now for 60.

@hashseed

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Just as a reminder, v8::Value::InstanceOf and v8::Symbol::GetHasInstance can now be used on master.

@hashseed

This comment has been minimized.

Copy link
Member

commented Oct 11, 2017

I'm working on a PR to use v8::Value::InstanceOf in napi.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 11, 2018
n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

PR-URL: nodejs#12246
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

PR-URL: nodejs#12246
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

PR-URL: nodejs#12246
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Apr 16, 2018
n-api: cache Symbol.hasInstance
This improves the performance of napi_instanceof() by retrieving
Symbol.hasInstance from the global object once and then storing a
persistent reference to it in the env.

Backport-PR-URL: #19447
PR-URL: #12246
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins referenced this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.