Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Merge N-API JSRT implementation from abi-stable-node #238

Merged
merged 4 commits into from
May 15, 2017

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented May 8, 2017

When Node-ChakraCore merged the latest changes from upstream node last week, it got the V8 N-API implementation. This change replaces that with a JSRT implementation, so that N-API calls do not go through the shim. The public API surface in node_api.h is unchanged.

This code is a result of a collaborative effort from the N-API working group in the api-prototype-chakracore-8.x branch of the abi-stable-node fork. It includes contributions from the following people in alphabetical order:

In addition to all the work done in the abi-stable-node fork, the following changes were done for this PR:

The N-API V8 implementation (node_api.cc) is not deleted here, but merely excluded from the build, so that there are no conflicts in future merges from upstream node.

Related links:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@jasongin jasongin requested a review from kunalspathak May 8, 2017 19:42
@boingoing
Copy link
Contributor

LGTM 👍

@kunalspathak
Copy link
Member

Copy JSRT weak-reference APIs from abi-stable-node fork

Are these changes in Microsoft/chakracore's 2.0 branch? If yes, can you just sync Microsoft/Chakracore branch 2.0 instead?

@@ -74,7 +74,8 @@ if (typeof Symbol !== 'undefined' && 'hasInstance' in Symbol &&
compareToNative(x, MySubClass);
compareToNative(y, MySubClass);
compareToNative(x, MyClass);
compareToNative(y, MyClass);
// TODO: https://github.com/nodejs/abi-stable-node/issues/236
Copy link
Member

Choose a reason for hiding this comment

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

TODO [](start = 5, length = 4)

nit: Should we open/port these issues to nodejs/node?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not, because these issues are specific to N-API on Node-ChakraCore. The tests pass with N-API on Node-V8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe they could be moved to nodejs/node-chakracore/issues. I think sometime soon we will stop using the abi-stable-node fork, now that all the code has been merged from it. But then we'll review all the remaining open issues there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to add the ChakraCore check rather than commenting these out?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kfarnung I'm not sure about that. These are issues that we want to fix in Node-ChakraCore, not engine-specific behavior that we want to allow indefinitely.

@jasongin
Copy link
Member Author

jasongin commented May 8, 2017

Copy JSRT weak-reference APIs from abi-stable-node fork

Are these changes in Microsoft/chakracore's 2.0 branch?

These JSRT APIs were added specifically to support N-API. Would it be better to add them to ChakraCore first, or do changes sometimes flow back from Node-ChakraCore?

@kfarnung
Copy link
Contributor

kfarnung commented May 8, 2017

@jasongin I've been making my ChakraCore changes in release/2.0 and then flowing them into node-chakracore. I don't believe we have a two-way sync, we just clone ChakraCore into node-chakracore.

@jasongin
Copy link
Member Author

jasongin commented May 9, 2017

I created a PR for the weak-reference APIs at chakra-core/ChakraCore#2948

I guess this PR should wait for that change to merge into ChakraCore and Node-ChakraCore.

@kunalspathak
Copy link
Member

I guess this PR should wait for that change to merge into ChakraCore and Node-ChakraCore.

That's correct.

node.gyp Outdated
'src/node_api.h',
'src/node_api_types.h',
# 'src/node_api.cc',
'src/node_api_jsrt.cc',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just add a "conditions" block which checked the engine type and then removes node_api.cc and adds in node_api_jsrt.cc?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll do that.

JsValueRef target;
CHECK_JSRT(JsGetWeakReferenceValue(weakRef, &target));

if (target == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be comparing against JS_INVALID_REFERENCE?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. (Though actually they are the same thing.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

dilijev added a commit to chakra-core/ChakraCore that referenced this pull request May 11, 2017
Merge pull request #2948 from jasongin:weakref

 - Add a new typedef: `JsWeakRef`
 - Add two new APIs: `JsCreateWeakReference()`, `JsGetWeakReferenceValue()`
 - Add a native test for the new APIs

The weak reference APIs are required for N-API in Node-ChakraCore. See nodejs/node-chakracore#238
chakrabot pushed a commit to chakra-core/ChakraCore that referenced this pull request May 11, 2017
Merge pull request #2948 from jasongin:weakref

 - Add a new typedef: `JsWeakRef`
 - Add two new APIs: `JsCreateWeakReference()`, `JsGetWeakReferenceValue()`
 - Add a native test for the new APIs

The weak reference APIs are required for N-API in Node-ChakraCore. See nodejs/node-chakracore#238
@@ -46,7 +46,8 @@ const str4 = '¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿';
assert.strictEqual(test_string.TestLatin1(str4), str4);
assert.strictEqual(test_string.TestUtf8(str4), str4);
assert.strictEqual(test_string.TestUtf16(str4), str4);
assert.strictEqual(test_string.TestLatin1Insufficient(str4), str4.slice(0, 3));
// TODO: https://github.com/nodejs/abi-stable-node/issues/235
// assert.strictEqual(test_string.TestLatin1Insufficient(str4), str4.slice(0, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

/home/kfarnung/GitHub/node-chakracore/test/addons-napi/test_string/test.js
50:1 error Line 50 exceeds the maximum line length of 80 max-len
61:1 error Line 61 exceeds the maximum line length of 80 max-len

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@kfarnung
Copy link
Contributor

Made some modifications to your original review and pushed to my fork:

https://github.com/kfarnung/node-chakracore/tree/napi

  • Enabled conditional sources for V8 and ChakraCore
  • Removed conflicting ChakraCore changes

@jasongin
Copy link
Member Author

jasongin commented May 15, 2017

Thanks @kfarnung! I rebased on top of the latest xplat (which now includes the JSRT weak-reference APIs), incorporated your changes in node.gyp, and addressed other review feedback above.

@kfarnung
Copy link
Contributor

kfarnung commented May 15, 2017

Copy link
Contributor

@kfarnung kfarnung left a comment

Choose a reason for hiding this comment

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

LGTM

jasongin and others added 4 commits May 15, 2017 11:26
 - Copy `node_api_jsrt.cc` from abi-stable-node fork
 - Update `node_api_jsrt.cc` for renamed JSRT string APIs:
   `JsCopyStringUtf8` -> `JsCopyString`, etc
 - Update `node.gyp` to build `node_api_jsrt.cc` instead of
   `node_api.cc`
 - Temporarily disable 2 N-API test cases with known failures

PR-URL: nodejs#238
Reviewed-By: Taylor Woll <taylor.woll@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs#238
Reviewed-By: Taylor Woll <taylor.woll@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs#238
Reviewed-By: Taylor Woll <taylor.woll@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs#238
Reviewed-By: Taylor Woll <taylor.woll@microsoft.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@kfarnung kfarnung merged commit 28cdc33 into nodejs:xplat May 15, 2017
@jasongin jasongin deleted the napi branch May 16, 2017 00:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants