Add JSRT weak reference APIs #2948

jasongin · 2017-05-09T19:37:41Z

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

msftclas · 2017-05-09T19:37:45Z

@jasongin,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

kfarnung · 2017-05-09T20:40:33Z

lib/Jsrt/Jsrt.cpp

+    VALIDATE_JSREF(weakRef);
+
+    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
+        Memory::RecyclerWeakReference<char>* recyclerWeakReference = reinterpret_cast<Memory::RecyclerWeakReference<char>*>(weakRef);


Can we add a break on this line to make it easier to read?

agarwal-sandeep · 2017-05-09T20:44:17Z

lib/Jsrt/Jsrt.cpp

@@ -2890,6 +2890,40 @@ CHAKRA_API JsSetPromiseContinuationCallback(_In_ JsPromiseContinuationCallback p
    /*allowInObjectBeforeCollectCallback*/true);
 }

+CHAKRA_API JsCreateWeakReference(
+    _In_ JsValueRef value,
+    _Out_ JsWeakRef* weakRef)


weakRef [](start = 21, length = 7)

PARAM_NOT_NULL check for out parameters in both APIs

kfarnung · 2017-05-09T20:55:14Z

lib/Jsrt/Jsrt.cpp

+    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
+        Memory::RecyclerWeakReference<char>* recyclerWeakReference = reinterpret_cast<Memory::RecyclerWeakReference<char>*>(weakRef);
+        *value = reinterpret_cast<JsValueRef>(recyclerWeakReference->Get());
+        return JsNoError;


Does it make sense to return a specific error code when the reference was already released? Having it return an invalid reference seems ok, I just wasn't sure whether this was consistent with other APIs.

/cc @liminzhu

I would not consider it a failure when the weak-referenced value is not available, so I don't think it should return an error code then.

That's true and consistent with other libraries, there just aren't many APIs in JSRT where the out param can be JS_INVALID_REFERENCE after a successful call. I think the documentation is pretty clear, so that should cover it.

agarwal-sandeep · 2017-05-09T20:55:47Z

lib/Jsrt/Jsrt.cpp

@@ -2890,6 +2890,40 @@ CHAKRA_API JsSetPromiseContinuationCallback(_In_ JsPromiseContinuationCallback p
    /*allowInObjectBeforeCollectCallback*/true);
 }

+CHAKRA_API JsCreateWeakReference(


JsCreateWeakReference [](start = 11, length = 21)

I think this API should not be callable during collection callbacks as this allocates memory.

I'll add a check for that.

MSLaguana · 2017-05-09T20:58:59Z

test/native-tests/test-weakref/sample.cpp

+    JsWeakRef weakRef;
+    FAIL_CHECK(JsCreateWeakReference(valueRef, &weakRef));
+
+    FAIL_CHECK(JsGetWeakReferenceValue(weakRef, &valueRef));


Would it make sense to fetch the value to a new reference, and check that valueRef == weakRefValue?

agarwal-sandeep · 2017-05-09T20:59:30Z

lib/Jsrt/Jsrt.cpp

+
+        Recycler* recycler = threadContext->GetRecycler();
+        Memory::RecyclerWeakReference<char>* recyclerWeakReference =
+            recycler->CreateWeakReferenceHandle<char>(reinterpret_cast<char*>(value));


CreateWeakReferenceHandle [](start = 22, length = 25)

Should it be ok to use FindOrCreateWeakReferenceHandle instead of CreateWeakReferenceHandle?

I'm not sure. I think @boingoing actually wrote this code originally. Taylor can you comment on this?

agarwal-sandeep · 2017-05-09T21:01:24Z

lib/Jsrt/Jsrt.cpp

@@ -2890,6 +2890,40 @@ CHAKRA_API JsSetPromiseContinuationCallback(_In_ JsPromiseContinuationCallback p
    /*allowInObjectBeforeCollectCallback*/true);
 }

+CHAKRA_API JsCreateWeakReference(
+    _In_ JsValueRef value,
+    _Out_ JsWeakRef* weakRef)


weakRef [](start = 21, length = 7)

Initialize weakRef to nullptr

agarwal-sandeep · 2017-05-09T21:02:20Z

lib/Jsrt/Jsrt.cpp

+
+CHAKRA_API JsGetWeakReferenceValue(
+    _In_ JsWeakRef weakRef,
+    _Out_ JsValueRef* value)


value [](start = 22, length = 5)

Initialize value to JS_INVALID_REFERENCE

MSLaguana · 2017-05-09T21:04:18Z

lib/Jsrt/Jsrt.cpp

+{
+    VALIDATE_JSREF(weakRef);
+
+    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {


Is GC deterministic? What would happen in a TTD scenario when the object backing a weak reference is collected at different points?

I don't know enough about how TTD works to answer that. I'd appreciate input from someone else about how weak references should interact with TTD.

@mrkmarron can hopefully offer an opinion when he's got the time

MSLaguana · 2017-05-09T21:09:39Z

Looks like the OSX and Ubuntu static tests failed your freshly added test; after invoking the GC the reference wasn't invalidated.

MSLaguana · 2017-05-09T21:14:56Z

@dotnet-bot test Ubuntu ubuntu_linux_release please

jasongin · 2017-05-09T21:38:19Z

Looks like the OSX static test failed your freshly added test; after invoking the GC the reference wasn't invalidated.

I see ubuntu_linux_debug_static failed the same way. I ran the test (and it passed) on Ubuntu with a release build. Maybe the GC behavior is different with a debug build? I'll try to repro.

jasongin · 2017-05-09T23:15:21Z

I confirmed the test passes with a Release build and fails with a Debug build. I assume because the GC has different configuration in a debug build. I don't know yet what to do about that.

jasongin · 2017-05-09T23:17:12Z

Is the test even in the right place? I just noticed there are other (more complete?) JSRT tests at bin/NativeTests/JsRTApiTest.cpp. But it looks like those are Windows-only? Should I move my test over to there?

kunalspathak · 2017-05-09T23:24:47Z

@jasongin - That should be the right place to add jsrt native test.

jasongin · 2017-05-10T16:56:42Z

When I put the test into bin/NativeTests/JsRTApiTest.cpp, it passes on Windows with both debug and release builds. But, those tests are Windows-only, is that OK?

Why is there native test code in 2 places, separated for Windows and non-Windows platforms?

kunalspathak · 2017-05-10T16:58:51Z

@akroshg - do you know why?

akroshg · 2017-05-10T17:02:38Z

Why not add test in core\bin\NativeTests ?

jasongin · 2017-05-10T17:07:40Z

@akroshg bin/NativeTests is what I just mentioned above. But those tests are Windows-only, aren't they? At least I don't see any way to build them on non-windows platforms. There is only a .vcxproj file.

I want to ensure the new APIs work on all supported platforms. Originally in this PR I had put tests under then test/native-tests directory, which seems a more obvious place to find tests than a bin directory.

akroshg · 2017-05-10T17:09:03Z

They were written in different branches. the windows only depends on the catch framework and during that time the cross-plat was not picked up. However there were compilation and other sort of problems happened when we directly use the windows to crossplat which needed to be looked at. (It just we haven't got time to consolidate them yet).

In reply to: 300545940 [](ancestors = 300545940)

akroshg · 2017-05-10T17:15:51Z

@ yes they are for windows only (although no effort made to consolidate them).

NativeTest were put along with ch under the bin directory. When I did that I put there so that all application (test or product) will be found under bin folder. But it seems like it can be confusing.

In reply to: 300549055 [](ancestors = 300549055)

jasongin · 2017-05-10T17:23:53Z

I pushed a couple commits to this PR, to address feedback above and move the test to bin/NativeTests.

Remaining issues are the question about FindOrCreateWeakReferenceHandle and concern about interaction with TTD

agarwal-sandeep · 2017-05-10T17:29:20Z

liminzhu · 2017-05-10T17:49:08Z

Do APIs get shipped to Chakra when in ChakraCommon.h? If that's the case might want to consider just putting the APIs in ChakraCore first in ChakraCore.h. Also curious how is this used in Node? Got kind of lost on nodejs/node-chakracore#238.

jasongin · 2017-05-10T18:02:36Z

Also curious how is this used in Node?

This is necessary to implement the reference functions in the new Node.js addon API in Node-ChakraCore. Those reference APIs are used by native addons that hold a reference to JavaScript values that have associated native data, such as a typed array with external data, or a native class instance that is "wrapped" in a JavaScript object. The weak references are useful because often the native code only needs to access the value as long as some JS code is still holding it.

In Node-V8, those APIs are implemented using a v8::Persistent, which supports weak-reference functionality via v8::PersistentBase::SetWeak().

liminzhu · 2017-05-10T18:33:51Z

@jasongin exactly what I look for, thanks! Do you imagine an additional finalizer param for JsCreateWeakReference called upon invalidating the weak reference useful, similar to what v8::PersistentBase::SetWeak() does?

jasongin · 2017-05-10T18:46:33Z

Do you imagine an additional finalizer param for JsCreateWeakReference called upon invalidating the weak reference useful, similar to what v8::PersistentBase::SetWeak() does?

That would be redundant because JsSetObjectBeforeCollectCallback already provides that functionality.

jasongin · 2017-05-10T18:49:07Z

Do APIs get shipped to Chakra when in ChakraCommon.h? If that's the case might want to consider just putting the APIs in ChakraCore first in ChakraCore.h

What does everyone think? While these APIs should work fine in Chakra, I don't know any reason they will be really needed there. (There's no plan to implement N-API for Node-Chakra AFAIK.)

liminzhu · 2017-05-10T19:22:49Z

@jasongin my argument - NAPI is still experimental and in case we need to change the new JSRT APIs for w/e reason it's a lot easier when they're just in ChakraCore.

I want them in Chakra eventually ofc. In general I don't like API differences between Chakra and ChakraCore unless there's legitimate reason not to ship something to Chakra, but there's no need to rush for the reason you mentioned. My mental model is to 'stage' experimental APIs in ChakraCore and once they mature we push them in Chakra.

- Move new APIs from ChakraCommon.h to ChakraCore.h, conditional on CHAKRACOREBUILD_ - Call FindOrCreateWeakReferenceHandle() instead of CreateWeakReferenceHandle()

jasongin · 2017-05-10T21:48:04Z

I pushed another commit:

Moved new APIs from ChakraCommon.h to ChakraCore.h, and made them conditional on CHAKRACOREBUILD_
Using FindOrCreateWeakReferenceHandle() instead of CreateWeakReferenceHandle()

jasongin · 2017-05-10T21:50:29Z

I think the TTD issue should not be blocking for now. These changes can only impact TTD if the new APIs are called, and in the short term that's only likely to be done via N-API in Node-ChakraCore, and N-API is still officially experimental.

MSLaguana · 2017-05-10T21:51:38Z

lib/Jsrt/JsrtCommonExports.inc

@@ -103,6 +103,8 @@
    JsGetRuntime
    JsIdle
    JsSetPromiseContinuationCallback
+    JsCreateWeakReference
+    JsGetWeakReferenceValue


Since you moved the headers to ChakraCore.h, you should also move these lines below the #ifndef below as well, I believe.

boingoing

LGTM 👍

I think FindOrCreateWeakReferenceHandle is the right thing to do here. Not sure about the TTD impact, though.

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

liminzhu · 2017-05-16T17:46:01Z

Added API reference in wiki. Please update them if needed in the future.

https://github.com/Microsoft/ChakraCore/wiki/JsWeakRef
https://github.com/Microsoft/ChakraCore/wiki/JsCreateWeakReference
https://github.com/Microsoft/ChakraCore/wiki/JsGetWeakReferenceValue

jasongin added 2 commits May 9, 2017 12:29

Add JSRT weak-reference APIs

deb0a4e

Add native test for weak references

671c390

jasongin requested a review from digitalinfinity May 9, 2017 19:37

msftclas added the cla-not-required label May 9, 2017

jasongin mentioned this pull request May 9, 2017

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

Merged

3 tasks

kfarnung reviewed May 9, 2017

View reviewed changes

agarwal-sandeep reviewed May 9, 2017

View reviewed changes

kfarnung reviewed May 9, 2017

View reviewed changes

agarwal-sandeep reviewed May 9, 2017

View reviewed changes

MSLaguana reviewed May 9, 2017

View reviewed changes

agarwal-sandeep reviewed May 9, 2017

View reviewed changes

kfarnung approved these changes May 9, 2017

View reviewed changes

agarwal-sandeep reviewed May 9, 2017

View reviewed changes

MSLaguana reviewed May 9, 2017

View reviewed changes

Address review feedback in weak-reference APIs

5ea344e

Move weak-reference test to JsRTApiTest.cpp

617966a

kfarnung approved these changes May 10, 2017

View reviewed changes

More review feedback for weak-reference APIs

042db69

- Move new APIs from ChakraCommon.h to ChakraCore.h, conditional on CHAKRACOREBUILD_ - Call FindOrCreateWeakReferenceHandle() instead of CreateWeakReferenceHandle()

MSLaguana reviewed May 10, 2017

View reviewed changes

Fix weakref exports conditional

7c2439e

boingoing approved these changes May 10, 2017

View reviewed changes

kfarnung approved these changes May 10, 2017

View reviewed changes

digitalinfinity approved these changes May 11, 2017

View reviewed changes

jasongin merged commit ca71319 into chakra-core:release/2.0 May 11, 2017

jasongin deleted the weakref branch May 11, 2017 06:02

Add JSRT weak reference APIs #2948

Add JSRT weak reference APIs #2948

Conversation

jasongin commented May 9, 2017

msftclas commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented May 9, 2017 • edited Loading

MSLaguana commented May 9, 2017

jasongin commented May 9, 2017

jasongin commented May 9, 2017

jasongin commented May 9, 2017 • edited Loading

kunalspathak commented May 9, 2017

jasongin commented May 10, 2017

kunalspathak commented May 10, 2017

akroshg commented May 10, 2017

jasongin commented May 10, 2017

akroshg commented May 10, 2017

akroshg commented May 10, 2017

jasongin commented May 10, 2017

agarwal-sandeep commented May 10, 2017

liminzhu commented May 10, 2017

jasongin commented May 10, 2017

liminzhu commented May 10, 2017

jasongin commented May 10, 2017

jasongin commented May 10, 2017

liminzhu commented May 10, 2017

jasongin commented May 10, 2017

jasongin commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

liminzhu commented May 16, 2017

MSLaguana commented May 9, 2017 •

edited

Loading

jasongin commented May 9, 2017 •

edited

Loading