Closed Bug 1098641 Opened 5 years ago Closed 5 years ago

Implement os.* functions for unix


(Core :: JavaScript Engine, defect)

Not set





(Reporter: sfink, Assigned: sfink)




(1 file)

This came about because I was using 'perf' for some benchmarking, but I kept running into version incompatibilities and differences between "perf record" vs "perf stat". So eventually I decided it would be better to eliminate the OS- and version-specific C++ code and just put in the necessary calls to enable doing it all within JS.

An example of the craziness: say you want to do 'perf stat', but just on a portion of a run. So you can't just do 'perf stat $JS ...', because it would cover everything. You can do 'perf stat -p <pid>' to start timing, but to stop you need to kill the perf process. And that only works with my current version. The previous version didn't report any stats if you killed it. So I had this horrific workaround where I spawned off a 'perf stat -p <js-pid> sleep 999999', then killed the 'sleep' process to mark the end of the timed region.

Anyway, it's wacky stuff, and changing rapidly. So it's problematic to compile it into the binary.
I don't think I actually needed all of these in the end, but they're all kind of related.
Attachment #8522587 - Flags: review?(jorendorff)
Comment on attachment 8522587 [details] [diff] [review]
Add os.{getpid,waitpid,spawn,system}

Review of attachment 8522587 [details] [diff] [review]:

Big review for a small patch. Sorry. r=me with changes.

::: js/src/shell/OSObject.cpp
@@ +84,5 @@
> +os_spawn(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.length() == 0) {

I noticed some functions, like this one, throw if you don't pass enough arguments; others throw if you don't pass exactly the right number of arguments. Please pick one and make this file (at least) consistent. The sloppy way is more JavaScripty.

@@ +104,5 @@
> +        return true;
> +    }
> +
> +    if (childPid == -1) {
> +        JS_ReportError(cx, "fork() failed");

Please take a sec and write a function for throwing exceptions when a system call fails and sets errno. It should use strerror for the error message and set errno to 0.

@@ +127,5 @@
> +    }
> +    if (!JS::ToInt32(cx, args[0], &pid))
> +        return false;
> +    int status = kill(pid, SIGINT);
> +    args.rval().setBoolean(status != -1);

Let's throw an exception here instead of returning false to JS. kill() sets errno too.

@@ +136,5 @@
> +os_waitpid(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() == 0 || args.length() > 2) {
> +        JS_ReportError(cx, "os.waitpid requires 1-2 arguments");

The first argument could default to -1, a wildcard.

@@ +154,5 @@
> +    if (result == -1) {
> +        JS_ReportError(cx, "os.waitpid failed");
> +        return false;
> +    }
> +    args.rval().setBoolean(status == pid);

This last line doesn't seem right. Maybe you mean result == pid. But doesn't the typical waitpid use case involve wanting to know the child's exit code at least?

Admittedly the waitpid API is kind of a train wreck. How about, on success, returning an object with a .pid property? That leaves the way open to add more information to that object later, like:

    if (WIFEXITED(status)) {
        if (!JS_DefineProperty(cx, obj, "exitStatus", WEXITSTATUS(status)))
            return false;

Or whatever turns out to make sense.

(Another object is to return an array [pid, status], and expose os.WIFEXITED and os.WEXITSTATUS as well, and while you're at it, os.WNOHANG. Python does this, but it seems silly in JS.)

@@ +166,5 @@
> +    JS_FS("system", os_system, 1, 0),
> +#ifndef XP_WIN
> +    JS_FS("spawn", os_spawn, 1, 0),
> +    JS_FS("kill", os_kill, 1, 0),
> +    JS_FS("waitpid", os_waitpid, 1, 0),

Now is a good time to switch this to JSFunctionSpecWithHelp and JS_DefineFunctionsWithHelp.
Attachment #8522587 - Flags: review?(jorendorff) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1108938
You need to log in before you can comment on or make changes to this bug.