619120 - Add a run function to the shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

WebKit changed Sunspider. Instead of calling load(), it calls run(), a function which returns the time in milliseconds. This means it breaks sunspider for us.

While it would be nice if they were a little more considerate of downstream, it seems easier to copy this than to complain. It also has the advantage that it can take the dateNow() feature I had in, to get more accurate results (which I think was what they were aiming for).

Comment 1

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

WIP - this builds and runs "Hello, world".  Haven't run on the latest sunspider yet.

Style nits etc welcome at this stage. I have lots of questions now that we are C++...

Also, how should we handle error cases in the shell for things like "mysteriously unable to stat() and open file"?  I am just returning JSVAL_VOID now; not sure if we want to clog up js.msg will shell-only basically-useless error messages.

Comment 2

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

I'm happy with this patch now.  PBiggar, are you a JS peer? Or at least able to r+ NPOTB patches?

Note to reviewer -- this patch implements run() quite differently from load(); we fault in pages rather than doing traditional I/O, and the script is inflated from 8 to 16 bits before passing to the lexer. This should more closely approximate what happens in the browser eliminate a bit of wall-clock jitter.  Page faulting and script inflation are not part of the time measurement.

Comment 3

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

Comment 4

[Paul Biggar]

Comment on attachment 497793 [details] [diff] [review]
Patch to add run() to js shell

I can't review this; I don't understand the NSPR stuff. bz suggested brendan for this; worst case he'll know a better reviewer.

Some small nits that I know will come up later:

+"run('foo.js')\n"
+"  Run the file named by the first argument, returning the number of\n"
+"  of milliseconds spent compiling and executing it",

Can you space this off to the right, in line with the line above:

 "load(['foo.js' ...])     Load files named by string arguments",
+"run('foo.js')            Run the file named by the first argument,\n"
+                          returning the number of\n"
+                          of milliseconds spent compiling"
+                          and executing it",


You've inconsistent use of JSBool with true/false and JS_TRUE/JS_FALSE. All return values should be the latter; non-return values should be the former (I mean the ok variable).

Comment 5

[Wesley W. Garland]

I'm going to leave these nits alone for the time being, as there are examples both ways for both things in this file. 

Will gladly fix if needed for r+

Comment 6

[Paul Biggar]

Comment on attachment 497793 [details] [diff] [review]
Patch to add run() to js shell

jason said he'd review this.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 497793 [details] [diff] [review]
Patch to add run() to js shell

>+    JSObject *thisobj = JS_THIS_OBJECT(cx, vp);
>+    if (!thisobj)
>+      return false;

Instead of this,

    JSObject *global = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp))->getGlobal();

It can't fail, and this way the this-value is just ignored, as I think it should be.

>+    jsval *argv = JS_ARGV(cx, vp);
>+    JSString *str = JS_ValueToString(cx, argv[0]);
>+    if (!str)
>+        return false;
>+    argv[0] = STRING_TO_JSVAL(str);

Writing this back to argv[0] isn't necessary anymore, but ok.

For reading the file, please factor out the important bits of Snarf into a new function and have both Snarf and Run use that.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 497793 [details] [diff] [review]
Patch to add run() to js shell

>+        ok = !compileOnly
>+             ? JS_ExecuteScript(cx, thisobj, script, NULL)
>+             : JS_TRUE;

compileOnly isn't declared anywhere.

Clearing review for now. Thanks for doing this!

Comment 9

[Paul Biggar]

(In reply to comment #7)
> For reading the file, please factor out the important bits of Snarf into a new
> function and have both Snarf and Run use that.

I think you mean that the elaborate NSPR stuff isn't necessary because we convert actually process the entire file during string conversion?

Comment 10

[Wesley W. Garland]

> For reading the file, please factor out the important bits of Snarf

I implemented Run() this way to try and reduce wall-clock jitter for the benchmarks -- once the warm-up pass goes, the kernel should only have to fault dirty pages back into the address space rather than re-doing I/O and re-building buffers.  Implementing Run() was intially in response to some discussions on IRC when we were talking about improving benchmark consistency.  Note that I haven't measured the effect on this benchmark/product, just using general past experience.

I can refactor three ways -- let me know which you'd like to see
1 - Both Snarf() and Run() always fault pages in are only !JS_THREADSAFE
2 - Both Snarf() and Run() fault pages in when JS_THREADSAFE
3 - Neither Snarf() nor Run() fault pages in

Faulting pages in (mmap) has really one disadvantage from my POV - reliance on NSPR. Buffer-building has always been a pet peeve. :)

> compileOnly isn't declared anywhere.

It's a global set by -C on the argument vector and used similarly by Load().  Hmm, I just realized, it's not possible to invoke Run() without executing script. Good catch!

Comment 11

[Brendan Eich [:brendan]]

Does WebKit's shell run method really time compilation? I'da thunk they wanted to approximate browser runtime timing by excluding compile time.

/be

Comment 12

[Paul Biggar]

(In reply to comment #10)
> > For reading the file, please factor out the important bits of Snarf
> 
> I implemented Run() this way to try and reduce wall-clock jitter for the
> benchmarks -- once the warm-up pass goes, the kernel should only have to fault
> dirty pages back into the address space rather than re-doing I/O and
> re-building buffers.  Implementing Run() was intially in response to some
> discussions on IRC when we were talking about improving benchmark consistency. 
> Note that I haven't measured the effect on this benchmark/product, just using
> general past experience.

Are the pages not faulted in during string conversion?


> It's a global set by -C on the argument vector and used similarly by Load(). 
> Hmm, I just realized, it's not possible to invoke Run() without executing
> script. Good catch!

I think this is probably acceptable, but feel free to make it work.

Comment 13

[Paul Biggar]

(In reply to comment #11)
> Does WebKit's shell run method really time compilation? I'da thunk they wanted
> to approximate browser runtime timing by excluding compile time.

My understanding of their code is that it does. Here's the code (from jsc.cpp):

EncodedJSValue JSC_HOST_CALL functionRun(ExecState* exec)
{   
    UString fileName = exec->argument(0).toString(exec);
    Vector<char> script;
    if (!fillBufferWithContentsOfFile(fileName, script))
        return JSValue::encode(throwError(exec, createError(exec, "Could not open file.")));

    GlobalObject* globalObject = new (&exec->globalData()) GlobalObject(Vector<UString>());

    StopWatch stopWatch;
    stopWatch.start();
    evaluate(globalObject->globalExec(), globalObject->globalScopeChain(), makeSource(script.data(), fileName));
    stopWatch.stop();
        
    return JSValue::encode(jsNumber(stopWatch.getElapsedMS()));
}

Comment 14

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

Re-factored to not use NSPR basically reuse snarf()'s guts.  "It compiles" -- will click the r? flag once I've tested it.

Comment 15

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

Comment 16

[Paul Biggar]

Comment on attachment 502829 [details] [diff] [review]
Patch to add run() to js shell

Run() needs a comment to say that we're duplicating the Apple run function especially for SunSpider, and also to say that all the IO should be done by the end of the call to JS_GetStringCharsAndLength.


+    str = FileAsString(cx, pathname);
     if (!str)
         return JS_FALSE;
+    JS_free(cx, (void*)pathname);
+


If |str| is null then we leak pathname. I would change the order here.

Comment 17

[Wesley W. Garland]

Attachment: Patch to add run() to js shell

Good catch on that core leak, Paul. Thanks!

Comment 18

[Igor Bukanov]

Attachment: Patch to add run() to js shell

I had to merge the patch as the previous version no longer applied as-is to the current tip. The patch is a must to have to run the SS from http://svn.webkit.org/repository/webkit/trunk/PerformanceTests/SunSpider . Also, since the run function uses sub-millisecond precision, the results varies less between runs even on a relatively busy system.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 508385 [details] [diff] [review]
Patch to add run() to js shell

Sorry for the delay.

Comment 20

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/33e1b18c10d0 - landed with extra fixes to silent rightful GCC warnings about use of not initialized variables.

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/33e1b18c10d0