Add build option for Node CLI by sebmarkbage · Pull Request #397 · facebookarchive/prepack

sebmarkbage · 2017-04-13T18:49:20Z

This adds a compatibility option for node-cli. The goal of this is to Prepack a whole Node.js CLI environment including Node's module system. This is useful for an end application but not so useful for a small library yet.

This takes a bit different approach than compiling a single bundle. Instead of running the target bundle in the Prepack realm, it actually runs Node.js's JavaScript code inside the Prepack realm. This code is embedded within the runtime and we can get to it through process.binding('natives') when we run Prepack.

The script file will be passed as a process.argv.

Node's CLI code will then pick that up and start loading modules from the file system. The idea is that reads from fs while Prepacking will be considered constant. That way all the relevant modules will be automatically picked up and executed in the Prepack realm.

Finally, we can snap shot the entire Node system.

This PR takes the approach of building intrinsics only for the native code in Node.js (which are all on the process object). An alternative solution would be to build intrinsics from the Node.js public APIs. The tradeoff is that the public API is much bigger surface area but moves slower, where as the native APIs are considered more private and therefore moves faster but have smaller surface area.

My guess is that it will be pretty easy to keep up with the native APIs but not the JS APIs so I picked that solution. It guarantees that the JS parts are 100% compatible with the Node version you're running.

This is still a WIP. I have more intrinsics to implement before this works.

sebmarkbage · 2017-04-13T18:51:25Z

@NTillmann This is the Call I was referring to. I want to do the equivalent of realm.$GlobalEnv.execute but call a function value that was the completion value of the previous execution. This is a very common way to initialize internals in JS engines/browsers in a way that doesn't leak those internals onto the global object (unlike React Native).

This seems reasonable to me. I think you should break it out into a separate function and co-located it with execute. Likewise, you may want to break out lines 176-197 and co-locate it with execute in environment.js.

NTillmann · 2017-04-13T21:39:13Z

How can this be? Do you mean value === null?

NTillmann · 2017-04-13T21:47:12Z

This exception catching for ...Completions seems dubious, but I can see how to copy&pasted it from evaluateCompletion. Let me check with Herman what should happen here.

Okay, here, you can collapse all of those ...Completions cases into a single "if (err instanceof Completion) res = err; else ...". This will log the issue in a reasonable way, and there is no way to recover at this point anyway.

It seems like call of this belongs in a helper somewhere but I'm not sure where.

NTillmann · 2017-04-13T22:01:13Z

We have realm.createErrorThrowCompletion to make this easier.

NTillmann · 2017-04-13T22:24:28Z

You filed in assume to make the "jsc" compatibility more precise by adding a version identifier. #395 Maybe. But then you probably also want to make "node"/"node-cli" versioned.

I didn't quite understand what "node" is. Looks like it is currently just for testing source maps. It probably should have some general "CommonJS" mode where you can't assume a particular version and many of the variables are abstract.

"node-cli" is interesting because I think we can make the versioning very automatic based on what node version you're running but then the environment is locked to that version of node. So it doesn't quite make sense to specify it.

sebmarkbage · 2017-04-13T23:19:00Z

Is it safe to hide this for the React Native builds or does it need it for some feature detection thing?

For now, let's just keep it under this compatibility flag.

hermanventer

I'm not quite sure I understand what you are trying to do here. What exactly does the bootstrap function do? Where does it get is input from? How do you designate input as unknown?

hermanventer · 2017-04-13T22:36:27Z

I'm none too sure about this comment. It seems to me that you are taking JS value and making a corresponding Prepack value.

hermanventer · 2017-04-14T02:37:57Z

This seems reasonable to me. I think you should break it out into a separate function and co-located it with execute. Likewise, you may want to break out lines 176-197 and co-locate it with execute in environment.js.

sebmarkbage · 2017-04-14T04:33:45Z

@hermanventer The bootstrap function will get its input from process.argv. Not yet implemented here. That's an array of the arguments passed on the command-line to the node executable.

The first argument is normally the JavaScript module that is going to be executed.

I was thinking that will be set up as a constant string value equal to the file about to be Prepacked. This will cause the Node environment to load that module file into memory via a call to the fs binding. That becomes the input source code.

The abstract input is the rest of the argv array, which lets the user pass arguments to their Prepacked node.js CLI.

Since the Node.js uses dynamic module source code loading as the program executes, it needs this different mechanism to be compatible with the ecosystem. Unlike browser/react native where bundles are created by statically analyzing dependency graphs.

sebmarkbage · 2017-04-14T04:35:02Z

Seeing what bootstrap_node does might be helpful: https://github.com/nodejs/node/blob/master/lib/internal/bootstrap_node.js#L146

hermanventer · 2017-04-14T17:40:56Z

The most I can figure out quickly from all this is that you might be able to prepack the bootstrap function. I'm wondering just how much value this would unlock. What am I missing?

sebmarkbage · 2017-04-14T17:45:34Z

@hermanventer That is correct. I still need to implement contextify and fs module. The next follow ups on the contextify stuff will demonstrate that. The contextify built-in will execute further scripts in the Prepack Realm when the bootstrap function calls it.

sebmarkbage · 2017-05-03T19:37:45Z

+      'transform-es2015-parameters',
+    ],
+    retainLines: true,
+  });


Note that this depends on a "devDependency" atm as a hack. I didn't want to add it as a hard dependency since this is still experimental and we'll just implement these directly eventually.

The purpose of this is to be able to land facebookarchive#397 with some test coverage. In its current form it is too specific in its implementation and only works on a specific version of Node. In the future we'll want to make that more abstract and general so that it can work on ranges and other major versions.

The purpose of this is to be able to land #397 with some test coverage. In its current form it is too specific in its implementation and only works on a specific version of Node. In the future we'll want to make that more abstract and general so that it can work on ranges and other major versions.

sebmarkbage · 2017-05-10T03:23:50Z

I added a unit test that is only enabled in CI for now (not yarn test) since it's locked to particular node version. This is now only blocked on #598 to be in a landable state (although far from complete).

sebmarkbage · 2017-05-10T05:20:22Z

I added an additional test case that tests the path where ArrayBuffer #598 is needed. CI should now fail until that's fixed.

sebmarkbage · 2017-05-13T02:48:25Z

After rebasing on top of the ArrayBuffer fix I think that this should now be in a landable state. After land, I'll follow up with Issues for a few ideas for future improvements.

hermanventer · 2017-05-18T01:45:37Z

@@ -0,0 +1,17 @@
+# Prepack test input
+node ./bin/prepack.js ./test/node-cli/Simple.js --out Simple-test.js --compatibility node-cli


How does prepack.js get into ./bin?

It's checked into the repo already. https://github.com/facebook/prepack/tree/master/bin

It's an alias for lib/prepack-cli.js which gets there from the build.

hermanventer · 2017-05-18T01:57:19Z

+
+declare var process: any;
+
+function getBufferFromArg(realm: Realm, value: Value): Uint8Array {


Just type value as an ObjectValue.

And add the invariant at each callsite instead?

Yes, that is actually preferable because it makes it easier for the reader to verify that the invariant is true.

hermanventer · 2017-05-18T01:58:25Z

+declare var process: any;
+
+function getBufferFromArg(realm: Realm, value: Value): Uint8Array {
+  invariant(value instanceof ObjectValue && value.$ViewedArrayBuffer);


This invariant is redundant.

hermanventer · 2017-05-18T01:59:55Z

+
+declare var process: any;
+
+function getBufferFromArg(realm: Realm, value: Value): Uint8Array {


You should probably use DataBlock instead of Uint8Array.

This is supposed to return a native Uint8Array in the Node environment. Not the Prepack value. It's what I pass to nativeFS.read etc. If the internal representation of DataBlock changes, I want that to be an error so that we can do a conversion from that format to Uint8Array here.

Fair enough, but it makes me wonder why we have DataBlock type in the first place.

hermanventer · 2017-05-18T02:01:51Z

+
+export default function (realm: Realm): ObjectValue {
+  let nativeBuffer = process.binding('buffer');
+  let nativeBufferPrototype = (require('buffer'): any).Buffer.prototype;


I don't understand what this is doing.

Hm. This is defined in native C code. This is related to the setupBufferJS thing below. The runtime we're currently running in has already called setupBufferJS, read from a mutated object and that's how it gets a handle on this object.

https://github.com/nodejs/node/blob/v7.9.0/src/node_buffer.cc#L1197-L1230

We can't do the same thing at this point. Luckily this same object is reachable another way through another module which I found here.

hermanventer · 2017-05-18T02:13:46Z

+    let utf8Slice = new NativeFunctionValue(realm, "Buffer.prototype.utf8Slice", "utf8Slice", 0, (context, args) => {
+      let self = getBufferFromArg(realm, context);
+      let decodedArgs = args.map((arg, i) => {
+        return ToInteger(realm, arg);


This seems like a good place to follow the arrow with an expression rather than a block with a return.

hermanventer · 2017-05-18T02:26:38Z

+      let decodedArgs = args.map((arg, i) => {
+        return ToInteger(realm, arg);
+      });
+      let utf8String = nativeBufferPrototype.utf8Slice.apply(self, decodedArgs);


I'm a bit surprised by this. The utf8Slice I can find does not expect a this parameter.

It's defined here: https://github.com/nodejs/node/blob/v7.9.0/src/node_buffer.cc#L1209

hermanventer · 2017-05-18T02:29:54Z

+      let self = getBufferFromArg(realm, context);
+      let decodedArgs = args.map((arg, i) => {
+        if (i === 0) {
+          return getBufferFromArg(realm, arg);


This seems inconsistent with utf8Slice and I can't think of a good reason why it should be.

utf8Slice creates a string from a Buffer. No side-effects. The rest of the arguments are all integers if they're defined.

copy copies bytes from this Buffer by mutating a target Buffer. The rest of the arguments are integers if defined. The return value is an integer.

https://github.com/nodejs/node/blob/v7.9.0/src/node_buffer.cc#L524-L557

hermanventer · 2017-05-18T02:32:48Z

+          return ToInteger(realm, arg);
+        }
+      });
+      let bytesCopied = nativeBufferPrototype.copy.apply(self, decodedArgs);


If you unpacked args with a pattern, you might get away with calling copy directly.

hermanventer · 2017-05-18T02:34:53Z

+    throw new Error("TODO");
+  });
+
+  Set(realm, obj, "setupBufferJS", setupBufferJS, true);


This would be more readable if moved to just after the definition of setupBufferJS.

hermanventer

I only really skimmed over this since it is quite large and somewhat strange to me. It seems self contained enough that I'm not overly concerned that it will break things, so I'm going to let you go ahead with this.

hermanventer · 2017-05-18T17:50:02Z

+
+declare var process: any;
+
+function getBufferFromArg(realm: Realm, value: Value): Uint8Array {


This seems like a duplicate of the one in buffer.js.

hermanventer · 2017-05-18T17:52:18Z

+    return realm.intrinsics.undefined;
+  });
+  obj.defineNativeMethod("internalModuleStat", 0, (context, args) => {
+    const fileName = ToString(realm, args[0]);


Rather use a pattern with more descriptive names.

hermanventer · 2017-05-18T17:57:13Z

+    enumerable: true,
+  });
+
+  // TODO


This comment can be a bit more descriptive. When your future self comes back it he might appreciate a hint as to what your present self had in mind.

hermanventer · 2017-05-18T17:58:29Z

+declare var process: any;
+
+// Takes a value from the host realm and create it into a Prepack Realm.
+// This seems like it could be a useful primitive elsewhere.


Perhaps move this to Realm.js.

hermanventer · 2017-05-18T18:01:20Z

+    enumerable: true,
+    configurable: true
+  });
+  // TODO


hermanventer · 2017-05-18T18:03:24Z

+    return realm.intrinsics.undefined;
+  });
+  TTYPrototype.defineNativeMethod("writeUtf8String", 0, (context, args) => {
+    // let req = args[0];


hermanventer · 2017-05-18T18:03:49Z

+    let fd = ToInteger(realm, args[0]);
+    return new StringValue(realm, nativeTTYWrap.guessHandleType(fd));
+    // TODO: Make this abstract so that changing the pipe at runtime is
+    // possible. Currently this cause an introspection error.


hermanventer · 2017-05-18T18:05:35Z

+    );
+    return realm.createAbstract(types, values, [], buildNode, undefined, `(process.binding('tty_wrap').isTTY(${fd}))`);
+  });
+  // TODO


hermanventer · 2017-05-18T18:08:04Z

+}
+
+export function prepackNodeCLISync(filename: string, options: Options = defaultOptions) {
+  if (process.version !== "v7.9.0") {


This seems rather specific. Wouldn't later versions work as well?

Not at the moment. The strategy is very fragile because I only model the minimal needed. Which is why this whole thing is so hacky. The ideal would be to have more of an automatic pass-through or making more things abstract.

To be clear, I'm modeling private APIs here. They do change between minor versions. The rationale for picking this strategy over modeling public APIs is outlined in the PR comment.

Expose `process` on intrinsics. We won't expose it on the global through the normal route. Instead, we'll let Node's on script set it up for us.

This lets us run from within a module scope for convenience.

This is just a sufficient amount of hacks to get the base line running. Needs more implementations and more of this needs to be automated.

I enable this mode only on CI for now. Not to `yarn test`. Since it is locked to a particular version of Node so we don't want to force people to run it locally.

The sync API doesn't accept a callback.

jdalton · 2017-07-29T15:30:20Z

I've noticed this may still throw errors when attempting to load built-in modules like "zlib".

sebmarkbage · 2017-07-29T17:37:29Z

@jdalton I think it'll be expected that many things won't work out of the box until the ecosystem shifts towards supporting "heap snapshot". E.g. Any hanging open socket at the end won't work 100%. We'll have to keep chasing the minimal set needed. Ideally Prepack can help minimize the things that can and can't be.

I'll take a look at why zlib doesn't work. Keep em coming. :)

jdalton · 2017-07-29T18:21:48Z

@sebmarkbage Cool, thanks!

sebmarkbage requested review from NTillmann and sebmck April 13, 2017 18:49

facebook-github-bot added the CLA Signed label Apr 13, 2017

sebmarkbage commented Apr 13, 2017

View reviewed changes

NTillmann reviewed Apr 13, 2017

View reviewed changes

sebmarkbage commented Apr 13, 2017

View reviewed changes

hermanventer reviewed Apr 14, 2017

View reviewed changes

sebmarkbage force-pushed the node-cli branch 5 times, most recently from e4303b3 to 1aa70c8 Compare April 20, 2017 03:01

sebmarkbage mentioned this pull request Apr 24, 2017

Bound Functions Referenced Before They're Initialized #449

Closed

sebmarkbage force-pushed the node-cli branch 2 times, most recently from a453fbe to 21b632d Compare April 26, 2017 00:04

sebmarkbage mentioned this pull request Apr 26, 2017

Move $ContextStack to $ErrorData, and append location data for syntax errors #461

Merged

sebmarkbage force-pushed the node-cli branch from 21b632d to 79b5582 Compare May 3, 2017 08:47

sebmarkbage commented May 3, 2017

View reviewed changes

sebmarkbage force-pushed the node-cli branch from 79b5582 to 70715a0 Compare May 4, 2017 03:05

sebmarkbage mentioned this pull request May 5, 2017

Update Circle CI Node Version to 7.9.0 #576

Merged

sebmarkbage force-pushed the node-cli branch from 70715a0 to a9c288a Compare May 9, 2017 06:13

sebmarkbage force-pushed the node-cli branch 2 times, most recently from 9634a7e to ef7448f Compare May 10, 2017 03:11

sebmarkbage force-pushed the node-cli branch from ef7448f to 94bbce2 Compare May 10, 2017 05:19

cblappert mentioned this pull request May 10, 2017

How to correctly define Node.js functions and properties for Prepack? #624

Closed

sebmarkbage force-pushed the node-cli branch 2 times, most recently from f0dcbe6 to c3c3261 Compare May 13, 2017 02:20

sebmarkbage changed the title ~~[WIP] Add build option for Node CLI~~ Add build option for Node CLI May 13, 2017

sebmarkbage mentioned this pull request May 15, 2017

Implement Automatic Proxy Between the Host JS and the Prepack JS Environment #644

Open

hermanventer reviewed May 18, 2017

View reviewed changes

hermanventer approved these changes May 18, 2017

View reviewed changes

sebmarkbage added 4 commits May 18, 2017 14:21

Add node-cli compatibility option

f6ce5c3

Expose `process` on intrinsics. We won't expose it on the global through the normal route. Instead, we'll let Node's on script set it up for us.

Use global instead of this to get the global in node-cli mode

8a1fbc1

This lets us run from within a module scope for convenience.

Implement some Node intrinsics

0b3f355

This is just a sufficient amount of hacks to get the base line running. Needs more implementations and more of this needs to be automated.

Add minimal test case for the node-cli mode

74b64c8

I enable this mode only on CI for now. Not to `yarn test`. Since it is locked to a particular version of Node so we don't want to force people to run it locally.

sebmarkbage force-pushed the node-cli branch from c3c3261 to 74b64c8 Compare May 18, 2017 22:09

Fix the CLI to actually output something

7bf406b

The sync API doesn't accept a callback.

sebmarkbage merged commit 1402638 into facebookarchive:master May 18, 2017

sebmarkbage mentioned this pull request Jun 23, 2017

Discussion/Tracking SnapshotCreator support nodejs/node#13877

Closed

14 tasks

		@@ -0,0 +1,17 @@
		# Prepack test input
		node ./bin/prepack.js ./test/node-cli/Simple.js --out Simple-test.js --compatibility node-cli


		declare var process: any;

		function getBufferFromArg(realm: Realm, value: Value): Uint8Array {

Conversation

sebmarkbage commented Apr 13, 2017

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!

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!

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!

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!

hermanventer left a comment

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!

sebmarkbage commented Apr 14, 2017

Uh oh!

sebmarkbage commented Apr 14, 2017

Uh oh!

hermanventer commented Apr 14, 2017

Uh oh!

sebmarkbage commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage commented May 10, 2017

Uh oh!

sebmarkbage commented May 13, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage May 18, 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!

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!

Choose a reason for hiding this comment

sebmarkbage commented May 10, 2017 •

edited

Loading

sebmarkbage May 18, 2017 •

edited

Loading

sebmarkbage May 18, 2017 •

edited

Loading