feat(unbundle): Adds unbundle support to react-native-windows #1017

rozele · 2017-02-17T18:40:11Z

Adds unbundle support by adding a check in ChakraJavaScriptExecutor for the UNBUNDLE magic file, and by adding a nativeRequire hook to the Chakra runtime to dynamically load files when needed. The intent is that this should reduce app start times by reducing the amount of JavaScript that needs to be initially processed, but it doesn't seem to have much effect on the simple Playground and UIExplorer apps I've tested it on.

Remaining TODO: add unbundle support to NativeJavaScriptExecutor

Depends on facebook/react-native#12423

Fixes #952

matthargett · 2017-02-17T18:51:53Z

ReactWindows/ReactNative.Shared/Common/IntegerHelpers.cs

+{
+    static class IntegerHelpers
+    {
+        public static uint LittleEndianToHost(uint value)


could we call this InetHelpers (or something similar) to reference the C header whose API we're emulating, and call the static method the same thing? #Closed

matthargett

Looks good to me, but it would be nice to see some tests. I love that this is all in Common code! 🥇

rozele · 2017-02-22T01:12:10Z

Thanks @matthargett, will see what I can do about tests once I get everything done. I still need to add support to the C++/CX ChakraBridge implementation. Once we get unbundles working there, it may make sense to revisit the PR to make that portable to WPF, as I anticipate that the indexed unbundle + memory mapped file may be the best solution in regards to app startup time.

rozele · 2017-02-23T18:11:38Z

Hoping that @mattpodwysocki will look at porting this logic to C++/CX ChakraBridge! #Closed

rozele · 2017-02-28T20:27:31Z

ReactWindows/ChakraBridge/ChakraHost.cpp

@@ -1,8 +1,13 @@
-#include "pch.h"
+#include <stdint.h>
+#include "pch.h"


Pedantic question: is there a prefered style for sorting <> includes "" includes? #Closed

Looks like <> comes after "" in other places.

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

rozele · 2017-02-28T20:29:03Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	if (argumentCount != 2)
+	{
+		return JS_INVALID_REFERENCE;
+	}


Just double checking, are we sure "no-op" is the correct behavior here? #Closed

Correct behavior is to throw exception.

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

rozele · 2017-02-28T20:34:31Z

ReactWindows/ChakraBridge/FileHelpers.cpp

+#include "pch.h"
+#include "FileHelpers.h"
+
+JsErrorCode FileHelpers::LoadFileContents(const wchar_t* szPath, wchar_t** pszData)


LoadFileContents [](start = 25, length = 16)

Let's just add statics to ChakraHost to reduce the diff for now.

rozele · 2017-02-28T20:44:17Z

ReactWindows/ChakraBridge/ChakraHost.cpp

-        CloseHandle(*hFile);
-        return JsErrorFatal;
-    }
+	*hMap = CreateFileMapping(*hFile, nullptr, PAGE_READONLY, 0, 0, L"ReactNativeMapping");


PAGE_READONLY [](start = 44, length = 13)

Parameterize these readonly changes. #Closed

mattpodwysocki · 2017-02-28T20:45:08Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+
+	// Copy the header data
+	uint32_t header[3];
+	CopyMemory(header, byteBuffer, sizeof(header));


Might want to use memcpy_s instead since it can trap error codes. #Closed

mattpodwysocki · 2017-02-28T20:45:43Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+	// Copy the script
+	auto script = new char[moduleData.length];
+	auto wcScript = new wchar_t[moduleData.length];
+	CopyMemory(script, byteBuffer + baseOffset + moduleData.offset, moduleData.length);


might want to use memcpy_s here instead which can now return an error code instead of throwing an AVE #WontFix

Good suggestion.

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

Throwing an exception is fine, it turns out that is the expected behavior in react-native for failures in the native callback.

In reply to: 103551729 [](ancestors = 103551729,103547834)

mattpodwysocki · 2017-02-28T20:47:15Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+	if (fileHandle != NULL)
+	{
+		UnmapViewOfFile(byteBuffer);
+		CloseHandle(mapHandle);


might want to trap bad actors of CloseHandle #WontFix

Just following the lead of https://github.com/ReactWindows/react-native-windows/blob/master/ReactWindows/ChakraBridge/SerializedSourceContext.h#L14-L24

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

Since all handles are generated from the same LoadByteCode function, I think the same assumptions apply that if the file handle is not null, then we can close these in the destructor successfully.

In reply to: 103551314 [](ancestors = 103551314,103548082)

mattpodwysocki · 2017-02-28T20:47:55Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+{
+	if (fileHandle != NULL)
+	{
+		UnmapViewOfFile(byteBuffer);


May want to trap if UnmapViewOfFile returns nonzero #WontFix

mattpodwysocki · 2017-02-28T20:48:14Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+	uint32_t numberOfTableEntries = header[1];
+	moduleTable = ModuleTable(numberOfTableEntries);
+	uint32_t moduleTableSize = moduleTable.byteLength();
+	CopyMemory(moduleTable.modules, byteBuffer + sizeof(header), moduleTableSize);


same concern with CopyMemory #Closed

Good suggestion here.

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

mattpodwysocki · 2017-02-28T20:48:26Z

ReactWindows/ChakraBridge/JsIndexedModulesUnbundle.cpp

+	uint32_t startupCodeSize = header[2];
+	auto startupCode = new char[startupCodeSize];
+	auto wcStartupCode = new wchar_t[startupCodeSize];
+	CopyMemory(startupCode, byteBuffer + baseOffset, startupCodeSize);


Same again with CopyMemory #Closed

mattpodwysocki · 2017-02-28T20:53:38Z

ReactWindows/ChakraBridge/JsModulesUnbundle.h

+public:
+    JsModulesUnbundle(const wchar_t* sourcePath);
+	~JsModulesUnbundle();
+	virtual JsModulesUnbundleModule* GetModule(uint32_t index);


would it be more useful if you had the JsModulesUnbundleModule as an out parameter and returned some error code? #WontFix

Can't we just use the nullptr as an error code?

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

I would agree except that these unbundles are being used in the context of a native callback, which is expected to return a JsValueRef. It's not clear how we could effectively transfer a JsErrorCode through the native callback back to the original call to JavaScript from the IJavaScriptExecutor.

In reply to: 103549854 [](ancestors = 103549854,103549241)

mattpodwysocki · 2017-02-28T20:54:01Z

ReactWindows/ChakraBridge/JsModulesUnbundle.h

+    JsModulesUnbundle(const wchar_t* sourcePath);
+	~JsModulesUnbundle();
+	virtual JsModulesUnbundleModule* GetModule(uint32_t index);
+	virtual wchar_t* GetStartupCode();


Same thing, would it be more useful to have an error code returned and an out param for the wchar_t? #Closed

In this case, yes.

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

rozele · 2017-02-28T20:59:02Z

ReactWindows/ChakraBridge/ModuleTable.cpp

+}
+
+
+ModuleTable::~ModuleTable()


ModuleTable [](start = 14, length = 11)

Delete unused code. #Closed

rozele · 2017-02-28T21:01:51Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	}
+
+	auto module = host->unbundle->GetModule((uint32_t)moduleId);
+	host->EvaluateScript(module->source, module->sourceUrl, nullptr);


source [](start = 30, length = 6)

Ensure module is not null. #Closed

rozele · 2017-02-28T21:02:57Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	}
+
+	auto module = host->unbundle->GetModule((uint32_t)moduleId);
+	host->EvaluateScript(module->source, module->sourceUrl, nullptr);


EvaluateScript [](start = 7, length = 14)

Do something with the error code. #Closed

rozele · 2017-02-28T21:03:50Z

ReactWindows/ChakraBridge/ChakraHost.cpp

    free(contents);

    return status;
 }

+JsErrorCode ChakraHost::EvaluateScript(const wchar_t* szScript, const wchar_t* szSourceUri, JsValueRef* result)
+{
+	return JsRunScript(szScript, currentSourceContext++, szSourceUri, result);


JsRunScript [](start = 8, length = 11)

Add nullptr checks for szScript and szSourceUri #Closed

mattpodwysocki · 2017-02-28T21:40:12Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	return JS_INVALID_REFERENCE;
+}
+
+BOOL IsUnbundle(const wchar_t* sourcePath)


you can use bool instead of BOOL here #Closed

mattpodwysocki · 2017-02-28T21:40:29Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	return magicHeader == MagicFileHeader;
+}
+
+BOOL IsIndexedUnbundle(const wchar_t* sourcePath)


Same, use bool instead of BOOL #Closed

mattpodwysocki · 2017-02-28T21:42:13Z

ReactWindows/ChakraBridge/ChakraHost.cpp

+	}
+
+	uint32_t magicHeader;
+	fread(&magicHeader, sizeof(uint32_t), 1, file);


might want to verify the size of the read return value #Closed

mattpodwysocki · 2017-02-28T21:43:26Z

ReactWindows/ChakraBridge/ChakraHost.cpp

@@ -187,7 +257,7 @@ JsErrorCode ChakraHost::RunSerializedScript(const wchar_t* szPath, const wchar_t
    }
    else
    {
-        IfFailRet(LoadByteCode(szSerializedPath, &buffer, &hFile, &hMap));
+        IfFailRet(LoadByteCode(szSerializedPath, &buffer, &hFile, &hMap, FALSE));


can use false here #Closed

rozele · 2017-03-09T20:46:17Z

Closing and reopening to trigger CLA bot

msftclas · 2017-03-09T20:46:22Z

@rozele,
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

rozele · 2017-03-10T22:19:03Z

To whom it may concern that may will merge this commit:

Please use Rebase and merge instead of Squash and merge. The set of 3 commits in this PR were purposefully crafted to work in isolation.

Each one is a separate feature.

codecov-io · 2017-07-17T12:28:12Z

Codecov Report

Merging #1017 into master will decrease coverage by 0.14%.
The diff coverage is 22.78%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
- Coverage   31.93%   31.78%   -0.15%     
==========================================
  Files         257      258       +1     
  Lines       18538    18675     +137     
  Branches     1566     1580      +14     
==========================================
+ Hits         5920     5936      +16     
- Misses      12470    12582     +112     
- Partials      148      157       +9

Impacted Files	Coverage Δ
...ctWindows/ReactNative.Shared/Bridge/ReactBridge.cs	`80% <0%> (ø)`	⬆️
...ve.Shared.Tests/Internal/MockJavaScriptExecutor.cs	`48.38% <0%> (ø)`	⬆️
...Shared/Chakra/Executor/ChakraJavaScriptExecutor.cs	`49.69% <22.29%> (-25.06%)`	⬇️
...ctWindows/ReactNative.Shared/Common/InetHelpers.cs	`50% <50%> (ø)`
...dows/ReactNative.Shared.Tests/Internal/AssertEx.cs	`84% <0%> (-4%)`	⬇️
...ts/Modules/Network/TaskCancellationManagerTests.cs	`93.7% <0%> (-0.7%)`	⬇️
...tNative.Shared.Tests/Internal/JavaScriptHelpers.cs	`96.36% <0%> (ø)`	⬆️
...s/Chakra/Executor/ChakraJavaScriptExecutorTests.cs	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cf8556...c4cf997. Read the comment docs.

Adds unbundle support by adding a check in ChakraJavaScriptExecutor for the UNBUNDLE magic file, and by adding a `nativeRequire` hook to the Chakra runtime to dynamically load files when needed. The intent is that this should reduce app start times by reducing the amount of JavaScript that needs to be initially processed, but it doesn't seem to have much effect on the simple Playground and UIExplorer apps I've tested it on. Remaining TODO: add unbundle support to NativeJavaScriptExecutor Depends on facebook/react-native#12423 Fixes microsoft#952

Indexed unbundle is a single-file approach to loading JavaScript modules. It is based off a single file with a binary header, including a module table with references to the offset and length of each module in the file.

It is desirable to use C++/CX for the unbundle feature because it supports memory mapped files, which will potentially reduce file IO.

matthargett reviewed Feb 17, 2017

View reviewed changes

rozele commented Feb 28, 2017

View reviewed changes

rozele force-pushed the issue952 branch 2 times, most recently from 9d77e1a to 686fc91 Compare February 28, 2017 20:43

rozele commented Feb 28, 2017

View reviewed changes

mattpodwysocki reviewed Feb 28, 2017

View reviewed changes

rozele commented Feb 28, 2017

View reviewed changes

rozele force-pushed the issue952 branch 2 times, most recently from da5bc7e to 1eddc04 Compare February 28, 2017 21:26

mattpodwysocki reviewed Feb 28, 2017

View reviewed changes

rozele force-pushed the issue952 branch 2 times, most recently from 102ed13 to ce0c07d Compare February 28, 2017 22:52

rozele force-pushed the issue952 branch from ce0c07d to 7b85391 Compare March 9, 2017 15:49

rozele closed this Mar 9, 2017

rozele reopened this Mar 9, 2017

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

rozele force-pushed the issue952 branch from 7b85391 to 0b27179 Compare March 10, 2017 22:16

rozele force-pushed the master branch from 71f4c38 to 26dcbd9 Compare June 24, 2017 00:29

rozele force-pushed the issue952 branch from 0b27179 to ccfefb5 Compare July 17, 2017 11:57

rozele added 3 commits July 27, 2017 19:36

feat(Unbundle): Adds indexed unbundle support

29a32f4

Indexed unbundle is a single-file approach to loading JavaScript modules. It is based off a single file with a binary header, including a module table with references to the offset and length of each module in the file.

feat(Unbundle): adds C++/CX support for Unbundle

c4cf997

It is desirable to use C++/CX for the unbundle feature because it supports memory mapped files, which will potentially reduce file IO.

rozele force-pushed the issue952 branch from ccfefb5 to c4cf997 Compare July 27, 2017 23:36

rozele merged commit dd5321d into microsoft:master Jul 28, 2017

rozele deleted the issue952 branch July 28, 2017 00:05

		}


		ModuleTable::~ModuleTable()

feat(unbundle): Adds unbundle support to react-native-windows #1017

feat(unbundle): Adds unbundle support to react-native-windows #1017

Uh oh!

Conversation

rozele commented Feb 17, 2017

Uh oh!

matthargett Feb 17, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthargett left a comment

Choose a reason for hiding this comment

Uh oh!

rozele commented Feb 22, 2017

Uh oh!

rozele commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rozele Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpodwysocki Feb 28, 2017 • edited by rozele Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

matthargett Feb 17, 2017 •

edited by rozele

Loading

rozele commented Feb 23, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

rozele Feb 28, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

rozele Feb 28, 2017 •

edited

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

mattpodwysocki Feb 28, 2017 •

edited by rozele

Loading

codecov-io commented Jul 17, 2017 •

edited

Loading