[NOT FOR CHECK IN] Design Feedback for PreparedJavaScriptStore #3650
Conversation
commit 1d3cdc8 Merge: 62fe483 a5f5255 Author: Yichen Yao <[email protected]> Date: Tue Nov 12 10:50:06 2019 -0800 Merge branch 'master' into RevampPreparedScriptStore commit 62fe483 Author: Yichen Yao <[email protected]> Date: Mon Nov 11 15:41:34 2019 -0800 Introduce ChakraRtRuntime. commit 9cbfa71 Author: Yichen Yao <[email protected]> Date: Mon Nov 11 14:32:21 2019 -0800 Finish up ChakraCoreRuntime. commit 77d1f11 Author: Yichen Yao <[email protected]> Date: Wed Nov 6 17:52:34 2019 -0800 Get rid of ScriptStore and refactor bytecode execution. commit 9acd7da Author: Yichen Yao <[email protected]> Date: Tue Nov 5 16:27:41 2019 -0800 Refactor ChakraRuntime. commit 6fd2719 Author: Yichen Yao <[email protected]> Date: Mon Nov 4 18:20:31 2019 -0800 Clean up ChakraRuntimeArgs.h commit 472b6fb Merge: c2a47e5 8b87988 Author: Yichen Yao <[email protected]> Date: Fri Nov 1 17:24:21 2019 -0700 Merge branch 'master' into RevampPreparedScriptStore commit c2a47e5 Author: Yichen Yao <[email protected]> Date: Fri Nov 1 17:18:37 2019 -0700 Introduce ChakraCoreRuntime.
| @@ -9,33 +9,45 @@ | |||
| </Filter> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)ByteArrayBuffer.h"> | |||
There was a problem hiding this comment.
[](start = 4, length = 66)
(nit) I will alphabetize this later. #Resolved
|
|
||
| assert( | ||
| static_cast<unsigned long long>(bytecodeSize) <= | ||
| static_cast<unsigned long long>((std::numeric_limits<size_t>::max)())); |
There was a problem hiding this comment.
There are plenty of places where we are checking for overflow/underflow before an integer conversion. It would be nice to implement and use something like boost::numeric_cast. In my opinion it would be nice to just use boost::numeric_cast instead of implementing our own. Thoughts? #Resolved
There was a problem hiding this comment.
We already have a dependency on boost. I would just use their version.
In reply to: 345396549 [](ancestors = 345396549)
| /** | ||
| * TODO (yicyao) | ||
| */ | ||
| class PreparedJavaScriptStore { |
There was a problem hiding this comment.
PreparedJavaScriptStore [](start = 6, length = 23)
I would appreciate some design feedback/suggestions here. Here are the problems that I find with the architecture proposed in this PR:
-
For now, let's assume that a user of ReactNative passes a RuntimeHolderLazyInit (see RuntimeHolder.h) as part of DevSettings. Consider a sample implementation of ChakraCoreJsiHolder. In order to construct a ChakraCoreRuntime, one need to be able to construct a ChakraCoreRuntimeArgs. In order to construct a ChakraCoreRuntimeArgs, one need to be able to construct a PreparedJavaScriptStore. A PreparedJavaScriptStore for ChakraCore will need to know about ChakraCorePreparedJavaScript. This means that we need to expose ChakraCoreJsiHolder, ChakraCoreRuntimeArgs, and ChakraCorePreparedJavaScript to the user of react native. This seems just bad.
-
Since every engine is different, each engine's PreparedJavaScript will likely be different, and for each engine, we might want different PreparedJavaScriptStore that cache/read PreparedJavaScript in different ways. We run into this combinatorial explosion. I think breaking this interface up into something that handles caching and reading and something that handles interpreting the structure of PreparedJavaScript may help with this. I am not sure where version checking would fit here though. #Resolved
There was a problem hiding this comment.
For #2, I agree that it feels like there are two components. Things like writing bytecode to disk would be shared between different runtimes, but generating information about things like versioning or creating the actual bytecode would be runtime specific.
For versioning, you could have the runtime give you something like a hash. E.g. for Chakra, you could combine all the normal fields about version with a Chakra specific magic number or something.
Maybe you could add an interface like PreparedScriptWriter that could output a buffer and a runtime hash?
In reply to: 345402558 [](ancestors = 345402558)
hansenyy
requested review from
a team,
NickGerleman,
tudorms,
mganandraj,
NikoAri and
vmoroz
| using ChakraRuntime::ChakraRuntime; | ||
| ~ChakraRtRuntime() noexcept = default; | ||
|
|
||
| #pragma region Functions_inherited_from_Runtime |
There was a problem hiding this comment.
#pragma region [](start = 0, length = 14)
nit: I think pragma region is useful for large spans of code, but it seems a bit overkill here. What about a comment that just says "Runtime" and "ChakraRuntime" above the block of functions? #Resolved
There was a problem hiding this comment.
I was originally using comments in ChakraRuntime.h until the span got too large. That's when I switch to #pragma region and ended up copying those here. I'll change these to comments.
In reply to: 345415851 [](ancestors = 345415851)
|
|
||
| std::unique_ptr<facebook::jsi::Runtime> makeChakraRtRuntime(const std::shared_ptr<ChakraRtRuntimeArgs> &args) noexcept { | ||
| auto runtime = std::make_unique<ChakraRtRuntime>(args); | ||
| runtime->Initialize(); |
There was a problem hiding this comment.
Initialize [](start = 11, length = 10)
Why don't we do this in the ctor if we want to do this after building anyway? The main place I've seen two-phase intiaalization be useful is for cases where we want to delay init because something is expensive, or offer flow control to handle failure. #ByDesign
There was a problem hiding this comment.
This would also let us get rid of the factory, unless we wanted it for a specific reason.
In reply to: 345416842 [](ancestors = 345416842)
There was a problem hiding this comment.
Initialize() need to invoke EvaluateJavaScriptSimple, a virtual function, on a bootstrap bundle for implementing HostObjects. Since virtual dispatch does not work in constructors, we need a separate Initialize step.
In reply to: 345417135 [](ancestors = 345417135,345416842)
|
It feels like there are multiple independent changes happening here. When we check this in, could we split it into separate PRs? #Resolved |
| private: | ||
| // We override the following because ChakraCore (unlike Chakra) offers | ||
| // weak reference semantics. | ||
| facebook::jsi::WeakObject createWeakObject(const facebook::jsi::Object &obj) override; |
There was a problem hiding this comment.
override [](start = 79, length = 8)
Should these be protected instead of private? #WontFix
There was a problem hiding this comment.
Nothing is inheriting from ChakraCoreRuntime right now so I don't think there is any reason for making members protected. If we need change this to protected in the future, we'll do it then.
In reply to: 345443262 [](ancestors = 345443262)
| private: | ||
| // Only ChakraCore supports weak reference semantics, so ChakraRtRuntime | ||
| // WeakObjects are in fact strong references. | ||
| facebook::jsi::WeakObject createWeakObject(const facebook::jsi::Object &obj) override; |
There was a problem hiding this comment.
override [](start = 79, length = 8)
protected? #WontFix
There was a problem hiding this comment.
|
|
||
| private: | ||
| // Only ChakraCore supports weak reference semantics, so ChakraRtRuntime | ||
| // WeakObjects are in fact strong references. |
There was a problem hiding this comment.
WeakObjects are in fact strong references [](start = 5, length = 41)
This worries me a lot, since it seems like we can silently keep objects alive too long and create leaks or hard to debug issues. Do we know if this is used? If not could we maybe terminate() or provide some behavior to make it clear something bad may happen? #Resolved
There was a problem hiding this comment.
This looks like existing behavior. Do we have a bug tracking this?
In reply to: 345444948 [](ancestors = 345444948)
There was a problem hiding this comment.
We do not have a but tracking this at the moment. I'll create a issue.
In reply to: 345450009 [](ancestors = 345450009,345444948)
|
|
||
| #pragma endregion Functions_inherited_from_ChakraRuntime | ||
|
|
||
| // Both sourceCode and sourceUrl must be UTF-16 encoded and null-terminated. |
There was a problem hiding this comment.
Both sourceCode and sourceUrl must be UTF-16 encoded and null-terminated. [](start = 5, length = 73)
nit: not sure if this is worth documenting. Seems implied already by the types. #Resolved
There was a problem hiding this comment.
|
It's a bit hard to review the chanegs from ChakraRuntime to ChakraCoreRuntime as is. In the future, would you be able to follow a pattern of
This lets the reviewer see diffs of what has actually changed. #Resolved |
|
Nevermind. It looks like putting this through a diff tool doesn't given anything too useful. In reply to: 553116750 [](ancestors = 553116750) |
|
|
||
| namespace Microsoft::JSI { | ||
|
|
||
| struct ChakraRuntimeArgs; | ||
|
|
||
| class ChakraPreparedJavaScript : public facebook::jsi::PreparedJavaScript { |
There was a problem hiding this comment.
ChakraPreparedJavaScript [](start = 6, length = 24)
nit: This header seems to be getting big. Would this be better in its own file? #Resolved
There was a problem hiding this comment.
| } | ||
|
|
||
| private: | ||
| std::string m_sourceUrl; |
There was a problem hiding this comment.
m_sourceUrl [](start = 14, length = 11)
nit: If we're going to offer these with direct access, should we just make them public and const? #Resolved
There was a problem hiding this comment.
That makes sense. Will make the appropriate change.
In reply to: 345453590 [](ancestors = 345453590)
There was a problem hiding this comment.
|
|
||
| // The returned std::shared_ptr<const facebook::jsi::PreparedJavaScript> | ||
| // points to a ChakraPreparedJavaScript and must only be used while this | ||
| // ChakraRuntime is still alive. |
There was a problem hiding this comment.
alive [](start = 28, length = 5)
I'm a bit confused by this comment. Would you be able to clarify? #Resolved
There was a problem hiding this comment.
In ChakraCoreRuntime,, the ChakraPreparedJavaScript returned by prepareJavaScript holds a ChakraCoreByteCodeBuffer, which in turns holds a ChakraObjectRef. (We do so to avoid copying bytecode from JS engined owned memory to RN owned memory.) So the returned ChakraPreparedJavaScript must only be used while the ChakraCoreRuntime that created it is still alive.
In reply to: 345454257 [](ancestors = 345454257)
| /** | ||
| * TODO (yicyao) | ||
| */ | ||
| std::unique_ptr<PreparedJavaScriptStore> m_preparedJsStore{nullptr}; |
There was a problem hiding this comment.
unique_ptr [](start = 7, length = 10)
Will we need to share the script store ever with something outside the runtime? Wondering if this should be ref counted. #WontFix
There was a problem hiding this comment.
I am not sure whether we need ref counting here or not. Let's keep this a unique_ptr for now and change it to a shared_ptr if needed in the future.
In reply to: 345456324 [](ancestors = 345456324)
| struct ChakraRuntimeArgs { | ||
| bool enableJITCompilation{true}; | ||
| public: |
There was a problem hiding this comment.
public [](start = 1, length = 6)
nit: public is implied for struct #WontFix
There was a problem hiding this comment.
I like to explicitly state the accessibly of members. If people feel strongly about this I can remove it.
In reply to: 345457882 [](ancestors = 345457882)
| /** | ||
| * @brief ChakraRuntime's policy for allocating executable pages. | ||
| */ | ||
| ExecutablePageAllocationPolicy m_executablePageAllocationPolicy{ExecutablePageAllocationPolicy::EnableAll}; |
There was a problem hiding this comment.
m_ [](start = 33, length = 2)
nit: I usually don't see the "m_" prefix for structs, only for private class members #Resolved
There was a problem hiding this comment.
I don't have strong preferences here so if people do not like the "m_" prefix I can remove them.
In reply to: 345458052 [](ancestors = 345458052)
| } | ||
|
|
||
| if (m_args->m_memoryTracker) { | ||
| std::shared_ptr<facebook::react::MemoryTracker> &memoryTracker = m_args->m_memoryTracker; |
There was a problem hiding this comment.
std::shared_ptr< [](start = 4, length = 16)
This is super super nitty, but this might be clearer as a ref to the underlying MemoryTracker instead of a ref to the shared_ptr. #Resolved
| @@ -560,7 +534,9 @@ facebook::jsi::Value ChakraRuntime::call( | |||
| std::vector<ChakraObjectRef> argRefs = ToChakraObjectRefs(args, count); | |||
|
|
|||
| std::vector<JsValueRef> argsWithThis = ConstructJsFunctionArguments(thisRef, argRefs); | |||
| assert(argsWithThis.size() <= (std::numeric_limits<unsigned short>::max)()); | |||
| assert( | |||
| static_cast<unsigned long long>(argsWithThis.size()) <= | |||
There was a problem hiding this comment.
unsigned long long [](start = 18, length = 18)
nit: I think "long" and "long long" can confuse folks occasionally. What about uint64_t? #Resolved
There was a problem hiding this comment.
Yes uint64_t works here. But then if we can use numeric_cast we can get rid of this altogether.
In reply to: 345460001 [](ancestors = 345460001)
| " return new Proxy(target, handler)\n" | ||
| "}"; | ||
|
|
||
| constexpr const char *const g_proxyConstructorBootstrapFuncName = "$$ChakraRuntimeProxyConstructor$$"; |
There was a problem hiding this comment.
g_proxyConstructorBootstrapFuncName [](start = 28, length = 35)
nit: we should keep these in their original position. #Resolved
There was a problem hiding this comment.
| */ | ||
| class PreparedJavaScriptStore { | ||
| public: | ||
| virtual ~PreparedJavaScriptStore() = 0; |
There was a problem hiding this comment.
0 [](start = 39, length = 1)
should this be default? #Pending
There was a problem hiding this comment.
|
Next time I'll try to do what you suggested here. In reply to: 553116750 [](ancestors = 553116750) |
|
For check in I will likely be splitting this change into two: one for introducing the class hierarchy, one for redesigning PreparedScriptStore. But I thought I would send out a change with both so that people can get a feel of the entire architecture. If this ended up being the opposite of helpful please let me know. In reply to: 553096653 [](ancestors = 553096653) |
|
|
||
| namespace Microsoft::JSI { | ||
|
|
||
| class ChakraCoreRuntime : public ChakraRuntime { |
There was a problem hiding this comment.
ChakraCoreRuntime [](start = 6, length = 17)
Please add comments why do we have ChakraRuntime and ChakraCoreRuntime. #Resolved
|
|
||
| #pragma once | ||
|
|
||
| #ifdef CHAKRACORE |
There was a problem hiding this comment.
CHAKRACORE [](start = 7, length = 10)
Add comment why this is needed
|
|
||
| namespace Microsoft::JSI { | ||
|
|
||
| class ChakraCoreRuntime : public ChakraRuntime { |
There was a problem hiding this comment.
ChakraCoreRuntime [](start = 6, length = 17)
Should this be final? #Resolved
|
|
||
| class ChakraCoreRuntime : public ChakraRuntime { | ||
| public: | ||
| ChakraCoreRuntime(const std::shared_ptr<ChakraCoreRuntimeArgs> &args) noexcept; |
There was a problem hiding this comment.
std::shared_ptr & [](start = 26, length = 40)
(nit) Since this is a constructor, would it make sense to pass by std::shared_ptr&&? #Resolved
|
|
||
| #pragma region Functions_inherited_from_ChakraRuntime | ||
|
|
||
| facebook::jsi::Value EvaluateJavaScriptSimple( |
There was a problem hiding this comment.
EvaluateJavaScriptSimple [](start = 23, length = 24)
Why "Simple"? Please add comment #Resolved
|
|
||
| std::string description() override; | ||
|
|
||
| private: |
There was a problem hiding this comment.
Why is this inside of inherited from Jsi Runtime region? #Resolved
| private: | ||
| // We override the following because ChakraCore (unlike Chakra) offers | ||
| // weak reference semantics. | ||
| facebook::jsi::WeakObject createWeakObject(const facebook::jsi::Object &obj) override; |
There was a problem hiding this comment.
createWeakObject [](start = 28, length = 16)
Similar to comments on other code reviews, can we do "MakeWeakObject"? this would be consistent with Mso::Make and std::make_*
Or this is overridden method we can't change? #ByDesign
There was a problem hiding this comment.
|
|
||
| using WeakChakraPointerValue = ChakraPointerValueTemplate<JsWeakRef>; | ||
|
|
||
| // source must be a JS ArrayBuffer or JS ExternalArrayBuffer. sourceUrl must |
There was a problem hiding this comment.
s [](start = 64, length = 1)
(nit) start new line #Resolved
|
|
||
| using WeakChakraPointerValue = ChakraPointerValueTemplate<JsWeakRef>; | ||
|
|
||
| // source must be a JS ArrayBuffer or JS ExternalArrayBuffer. sourceUrl must |
There was a problem hiding this comment.
// [](start = 2, length = 2)
(nit) please use doxygen style comments, eg //!
|
|
||
| // In-proc debugging helpers | ||
| void ProcessDebuggerCommandQueue(); | ||
| static void CALLBACK ProcessDebuggerCommandQueueCallback(void *callbackState); |
There was a problem hiding this comment.
CALLBACK [](start = 14, length = 8)
OOC why is this needed? #Resolved
There was a problem hiding this comment.
We need to pass a callback of this signature to ChakraCoreDebugger API when setting up direct debugging.
In reply to: 345520533 [](ancestors = 345520533)
|
|
||
| // In-proc debugging helpers | ||
| void ProcessDebuggerCommandQueue(); | ||
| static void CALLBACK ProcessDebuggerCommandQueueCallback(void *callbackState); |
There was a problem hiding this comment.
void * [](start = 59, length = 6)
Can't we pass something else besides void*? If this is id, it could be intptr_t #Resolved
There was a problem hiding this comment.
This function and all the other functions with the CALLBACK macro prefix are passed to ChakraCore APIs, as a result their signatures can't be changed.
In reply to: 345520788 [](ancestors = 345520788)
|
|
||
| // Promise Helpers | ||
| void PromiseContinuation(ChakraObjectRef &&value) noexcept; | ||
| void PromiseRejectionTracker(ChakraObjectRef &&promise, ChakraObjectRef &&reason, bool handled); |
There was a problem hiding this comment.
); [](start = 96, length = 2)
should this be noexcept too? What about other functions? #Resolved
There was a problem hiding this comment.
This actually should not be noexcept. CharkCore API requires a pointer to a non-noexcept function. It is strange to me that this was able to compile.
In reply to: 345520983 [](ancestors = 345520983)
| void PromiseRejectionTracker(ChakraObjectRef &&promise, ChakraObjectRef &&reason, bool handled); | ||
| static void CALLBACK PromiseContinuationCallback(JsValueRef funcRef, void *callbackState) noexcept; | ||
| static void CALLBACK | ||
| PromiseRejectionTrackerCallback(JsValueRef promise, JsValueRef reason, bool handled, void *callbackState); |
There was a problem hiding this comment.
(nit) can you reformat this to something like:
static void CALLBACK PromiseRejectionTrackerCallback(
JsValueRef promise, JsValueRef reason, bool handled, void *callbackState); #Resolved
| static void CALLBACK | ||
| PromiseRejectionTrackerCallback(JsValueRef promise, JsValueRef reason, bool handled, void *callbackState); | ||
|
|
||
| std::shared_ptr<ChakraCoreRuntimeArgs> m_args{nullptr}; |
There was a problem hiding this comment.
(nit) please add another private: here to denote start of listing of variables #Resolved
|
|
||
| std::shared_ptr<ChakraCoreRuntimeArgs> m_args{nullptr}; | ||
|
|
||
| std::shared_ptr<DebugProtocolHandler> m_debugProtocolHandler{nullptr}; |
There was a problem hiding this comment.
nullptr [](start = 63, length = 7)
shared_ptr is null by default, right? so these should not be needed. #Resolved
|
So, I've also seen valid doxygen in the project using "///" or Javadoc style doxygen (E.g. DevSettings). Any thoughts on the one true comment style?
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Nikolai Aristov <[email protected]>
Sent: Tuesday, November 12, 2019 4:45:09 PM
To: microsoft/react-native-windows <[email protected]>
Cc: Nick Gerleman <[email protected]>; Review requested <[email protected]>
Subject: [EXTERNAL] Re: [microsoft/react-native-windows] [NOT FOR CHECK IN] Design Feedback for PreparedJavaScriptStore (#3650)
@NikoAri commented on this pull request.
________________________________
In vnext/JSI/Shared/ChakraCoreRuntime.h<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Freact-native-windows%2Fpull%2F3650%23discussion_r345520097&data=02%7C01%7CNicholas.Gerleman%40microsoft.com%7C61ef0dafc88445668b5b08d767d2bc83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637092027116115617&sdata=Rs%2FZx04Z6W4iabCqoq2x4yJ6buzf1b%2B6L9EvOgwdJbc%3D&reserved=0>:
+ facebook::jsi::Value EvaluateJavaScriptSimple(
+ const std::shared_ptr<const facebook::jsi::Buffer> &buffer,
+ const std::string &sourceUrl) override;
+
+ std::shared_ptr<const facebook::jsi::Buffer> SerializeScript(
+ const std::shared_ptr<const facebook::jsi::Buffer> &buffer) override;
+
+ void StartDebugging() override;
+
+ void InitializeAdditionalFeatures() override;
+
+#pragma endregion Functions_inherited_from_ChakraRuntime
+
+ using WeakChakraPointerValue = ChakraPointerValueTemplate<JsWeakRef>;
+
+ // source must be a JS ArrayBuffer or JS ExternalArrayBuffer. sourceUrl must
// [](start = 2, length = 2)
(nit) please use doxygen style comments, eg //!
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Freact-native-windows%2Fpull%2F3650%3Femail_source%3Dnotifications%26email_token%3DAAGL5E2WWWHT3BDYKEEC6TDQTNEZLA5CNFSM4JMHYEAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLKNQLA%23pullrequestreview-315938860&data=02%7C01%7CNicholas.Gerleman%40microsoft.com%7C61ef0dafc88445668b5b08d767d2bc83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637092027116115617&sdata=B5FhVLI5FW9fQf82dVbI6pueNubtdjTS%2FbJStqd4yU4%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAGL5EZKQSWU5MNAEODXHHDQTNEZLANCNFSM4JMHYEAA&data=02%7C01%7CNicholas.Gerleman%40microsoft.com%7C61ef0dafc88445668b5b08d767d2bc83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637092027116125613&sdata=Ddai3UyexBtHMDCo1CikF148LWQdyR5iXrveJqXeg1E%3D&reserved=0>.
|
| std::shared_ptr<DebugProtocolHandler> m_debugProtocolHandler{nullptr}; | ||
| std::unique_ptr<DebugService> m_debugService{nullptr}; | ||
|
|
||
| std::shared_ptr<bool> m_runtimeIsAlive{nullptr}; |
There was a problem hiding this comment.
shared_ptr [](start = 7, length = 10)
Why do we need a shared_ptr for this? #Resolved
There was a problem hiding this comment.
When native promise is turned on, we schedule promise continuation callbacks onto the jsQueue. When those callbacks are invoked, we need to check that the ChakraCore runtime has not been disposed before calling the JS promise continuation function. We take a weak_ptr to this member and use it to perform the runtime aliveness check.
In reply to: 345521748 [](ancestors = 345521748)
| /** | ||
| * TODO (yicyao) | ||
| */ | ||
| uint8_t *GetArrayBufferData(const ChakraObjectRef &arrBuf); |
There was a problem hiding this comment.
GetArrayBufferData [](start = 9, length = 18)
This api should include getting length as well
| * TODO (yicyao) | ||
| */ | ||
| class PreparedJavaScriptStore { | ||
| public: |
There was a problem hiding this comment.
public [](start = 1, length = 6)
I would recommend creating a small test which takes advantage of this Api. It could become much easier to spot which options should be exposed to the user.
ghost
added
the
Needs: Author Feedback
ghost
removed
the
Needs: Author Feedback
This PR is intended as a way to get some design feedback while I working on redesigning the PreparedJavaScriptStore (used to be called PreparedScriptStore). The only projects building at the moment are the JSI projects (except for JSI.Desktop.UnitTests).
Iteration 1:
Make Chakra and ChakraCore based jsi::Runtimes into a proper class hierarchy. ChakraCoreRuntime is the jsi::Runtime based on ChakraCore, ChakraRtRuntime is the jsi::Runtime based on Chakra, and both jsi::Runtimes derive from CharkraRuntime. The name "ChakraRtRuntime" comes from the header that gets #included by jsrt.h when USE_EDGEMODE_JSRT is #defined.
There are still some macro guarding in code. Most of them are in ChakraObjectRef.h/cpp and all have comments explaining why they are there. The rest are used to make sure that, when a specific engine is used, only the corresponding jsi::Runtime is available.
I also tried to simplify the PreparedJavaScriptStore interface.Originally I wanted to also supply a prototype implementation of PreparedJavaScriptStore as part of iteration 1, but I ran into some design problem here. I would very much appreciate some feedback here.
Microsoft Reviewers: Open in CodeFlow