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.

Sign up for GitHub

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

Jump to bottom

Add JSRT weak reference APIs #2948

Merged
merged 6 commits into from
May 11, 2017
Merged

Add JSRT weak reference APIs #2948

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
deb0a4e
Add JSRT weak-reference APIs
jasongin May 8, 2017
671c390
Add native test for weak references
jasongin May 9, 2017
5ea344e
Address review feedback in weak-reference APIs
jasongin May 10, 2017
617966a
Move weak-reference test to JsRTApiTest.cpp
jasongin May 10, 2017
042db69
More review feedback for weak-reference APIs
jasongin May 10, 2017
7c2439e
Fix weakref exports conditional
jasongin May 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions bin/NativeTests/JsRTApiTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,37 @@ namespace JsRTApiTest
JsRTApiTest::RunWithAttributes(JsRTApiTest::ReferenceCountingTest);
}

void WeakReferenceTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
{
JsValueRef valueRef = JS_INVALID_REFERENCE;
REQUIRE(JsCreateString("test", strlen("test"), &valueRef) == JsNoError);

JsWeakRef weakRef = JS_INVALID_REFERENCE;
REQUIRE(JsCreateWeakReference(valueRef, &weakRef) == JsNoError);

// JsGetWeakReferenceValue should return the original value reference.
JsValueRef valueRefFromWeakRef = JS_INVALID_REFERENCE;
CHECK(JsGetWeakReferenceValue(weakRef, &valueRefFromWeakRef) == JsNoError);
CHECK(valueRefFromWeakRef != JS_INVALID_REFERENCE);
CHECK(valueRefFromWeakRef == valueRef);

// Clear the references on the stack, so that the value will be GC'd.
valueRef = JS_INVALID_REFERENCE;
valueRefFromWeakRef = JS_INVALID_REFERENCE;

CHECK(JsCollectGarbage(runtime) == JsNoError);

// JsGetWeakReferenceValue should return an invalid reference after the value was GC'd.
JsValueRef valueRefAfterGC = JS_INVALID_REFERENCE;
CHECK(JsGetWeakReferenceValue(weakRef, &valueRefAfterGC) == JsNoError);
CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);
}

TEST_CASE("ApiTest_WeakReferenceTest", "[ApiTest]")
{
JsRTApiTest::RunWithAttributes(JsRTApiTest::WeakReferenceTest);
}

void ObjectsAndPropertiesTest1(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
{
JsValueRef object = JS_INVALID_REFERENCE;
Expand Down
37 changes: 37 additions & 0 deletions lib/Jsrt/ChakraCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,16 @@ typedef unsigned short uint16_t;
/// </remarks>
typedef JsRef JsPropertyIdRef;

/// <summary>
/// A weak reference to a JavaScript value.
/// </summary>
/// <remarks>
/// A value with only weak references is available for garbage-collection. A strong reference
/// to the value (<c>JsValueRef</c>) may be obtained from a weak reference if the value happens
/// to still be available.
/// </remarks>
typedef JsRef JsWeakRef;

/// <summary>
/// Attributes of a runtime.
/// </summary>
Expand Down Expand Up @@ -2390,6 +2400,33 @@ typedef unsigned short uint16_t;
_In_ JsPromiseContinuationCallback promiseContinuationCallback,
_In_opt_ void *callbackState);

/// <summary>
/// Creates a weak reference to a value.
/// </summary>
/// <param name="value">The value to be referenced.</param>
/// <param name="weakRef">Weak reference to the value.</param>
/// <returns>
/// The code <c>JsNoError</c> if the operation succeeded, a failure code otherwise.
/// </returns>
CHAKRA_API
JsCreateWeakReference(
_In_ JsValueRef value,
_Out_ JsWeakRef* weakRef);

/// <summary>
/// Gets a strong reference to the value referred to by a weak reference.
/// </summary>
/// <param name="weakRef">A weak reference.</param>
/// <param name="value">Reference to the value, or <c>JS_INVALID_REFERENCE</c> if the value is
/// no longer available.</param>
/// <returns>
/// The code <c>JsNoError</c> if the operation succeeded, a failure code otherwise.
/// </returns>
CHAKRA_API
JsGetWeakReferenceValue(
_In_ JsWeakRef weakRef,
_Out_ JsValueRef* value);

#ifdef _WIN32
#include "ChakraCommonWindows.h"
#endif // _WIN32
Expand Down
52 changes: 48 additions & 4 deletions lib/Jsrt/Jsrt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ CHAKRA_API JsSetCurrentContext(_In_ JsContextRef newContext)
}
else
{
if(oldScriptContext->IsTTDRecordModeEnabled())
if(oldScriptContext->IsTTDRecordModeEnabled())
{
//already know newScriptContext != oldScriptContext so don't check again
if(oldScriptContext->ShouldPerformRecordAction())
Expand Down Expand Up @@ -2890,6 +2890,50 @@ CHAKRA_API JsSetPromiseContinuationCallback(_In_ JsPromiseContinuationCallback p
/*allowInObjectBeforeCollectCallback*/true);
}

CHAKRA_API JsCreateWeakReference(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a check for that.

_In_ JsValueRef value,
_Out_ JsWeakRef* weakRef)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

PARAM_NOT_NULL check for out parameters in both APIs

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Initialize weakRef to nullptr

{
VALIDATE_JSREF(value);
PARAM_NOT_NULL(weakRef);
*weakRef = nullptr;

return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
ThreadContext* threadContext = ThreadContext::GetContextForCurrentThread();
if (threadContext == nullptr)
{
return JsErrorNoCurrentContext;
}

Recycler* recycler = threadContext->GetRecycler();
if (recycler->IsInObjectBeforeCollectCallback())
{
return JsErrorInObjectBeforeCollectCallback;
}

Memory::RecyclerWeakReference<char>* recyclerWeakReference =
recycler->CreateWeakReferenceHandle<char>(reinterpret_cast<char*>(value));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Should it be ok to use FindOrCreateWeakReferenceHandle instead of CreateWeakReferenceHandle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*weakRef = reinterpret_cast<JsWeakRef>(recyclerWeakReference);
return JsNoError;
});
}

CHAKRA_API JsGetWeakReferenceValue(
_In_ JsWeakRef weakRef,
_Out_ JsValueRef* value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Initialize value to JS_INVALID_REFERENCE

{
VALIDATE_JSREF(weakRef);
PARAM_NOT_NULL(value);
*value = JS_INVALID_REFERENCE;

return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Memory::RecyclerWeakReference<char>* recyclerWeakReference =
reinterpret_cast<Memory::RecyclerWeakReference<char>*>(weakRef);
*value = reinterpret_cast<JsValueRef>(recyclerWeakReference->Get());
return JsNoError;
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 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

});
}

JsErrorCode RunScriptCore(JsValueRef scriptSource, const byte *script, size_t cb,
LoadScriptFlag loadScriptFlag, JsSourceContext sourceContext,
const WCHAR *sourceUrl, bool parseOnly, JsParseScriptAttributes parseAttributes,
Expand Down Expand Up @@ -3904,7 +3948,7 @@ CHAKRA_API JsTTDPreExecuteSnapShotInterval(_In_ JsRuntimeHandle runtimeHandle, _
return inflateStatus;
}

//If we are in the "active" segment set the continue breakpoint
//If we are in the "active" segment set the continue breakpoint
if((moveMode & JsTTDMoveMode::JsTTDMoveScanIntervalForContinueInActiveBreakpointSegment) == JsTTDMoveMode::JsTTDMoveScanIntervalForContinueInActiveBreakpointSegment)
{
GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode
Expand Down Expand Up @@ -3944,7 +3988,7 @@ CHAKRA_API JsTTDPreExecuteSnapShotInterval(_In_ JsRuntimeHandle runtimeHandle, _
elog->PopMode(TTD::TTDMode::DebuggerLogBreakpoints);
elog->PopMode(TTD::TTDMode::DebuggerSuppressBreakpoints);

//If we are in the "active" segment un-set the continue breakpoint
//If we are in the "active" segment un-set the continue breakpoint
if((moveMode & JsTTDMoveMode::JsTTDMoveScanIntervalForContinueInActiveBreakpointSegment) == JsTTDMoveMode::JsTTDMoveScanIntervalForContinueInActiveBreakpointSegment)
{
GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode
Expand Down Expand Up @@ -4036,7 +4080,7 @@ CHAKRA_API JsTTDReplayExecution(_Inout_ JsTTDMoveMode* moveMode, _Out_ int64_t*
return JsNoError;
});

if(bpstatus != JsNoError)
if(bpstatus != JsNoError)
{
return bpstatus;
}
Expand Down
2 changes: 2 additions & 0 deletions lib/Jsrt/JsrtCommonExports.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@
JsGetRuntime
JsIdle
JsSetPromiseContinuationCallback
JsCreateWeakReference
JsGetWeakReferenceValue
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

#ifndef NTBUILD
JsCreateString
JsCreateStringUtf16
Expand Down