411365 - Add JS function to start and stop Shark programatically.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Robert Sayre]

It looks like we can much cleaner profiles by starting and stopping the Shark inside JS scripts.

For example, the SunSpider bitwise-and test can be instrumented this way:

startShark();
bitwiseAndValue = 4294967296;
for (var i = 0; i < 600000; i++)
  bitwiseAndValue = bitwiseAndValue & i;
stopShark();

This feature requires the updated CHUD framework from http://developer.apple.com/tools/download/.

To try it out, start Shark, enable programatic sampling, and start the shell with "-k".

Comment 1

[Robert Sayre]

Attachment: add Shark functions to js.c

I should add that I cribbed the CHUD incantations from a similar patch that bz wrote for nsJSEnvironment.

Comment 2

[Robert Sayre]

Attachment: screenshot of fully-expanded bitwise-and profile

Comment 3

[Wesley W. Garland]

The change to Makefile.ref uses #ifdef .. #endif instead of ifdef...endif to mask out -DSHARK.  This has the effect of making -DSHARK the default on all platforms so needs to be fixed.

Note also, this doesn't build on my stock-ish Leopard, probably because there are no header files in 
/System/Library/PrivateFrameworks/CHUD.framework/, which are included when -DSHARK.   If this lands, it may be worth documenting somewhere what needs to be installed to build Spidermonkey with Shark support.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Note also, this doesn't build on my stock-ish Leopard,

See comment 0, the "This feature requires" part.

Comment 5

[Robert Sayre]

Attachment: correct ifdef thinko

Comment 6

[Brian Crowder]

Comment on attachment 296203 [details] [diff] [review]
correct ifdef thinko

+        case 'k':
+            /* Enable a profiler if available */
+#ifdef SHARK
+            gSharkEnabled = JS_TRUE;
+            chudInitialize();
+            chudAcquireRemoteAccess();
+#endif
+            break;

Better to preprocessor guard the whole case, I think, rather than silently ignore the flag.

+void StartChudRemote() {
+    if (gSharkEnabled) {
+        printf("Beginning shark profiling.\n");
+        chudStartRemotePerfMonitor("SpiderMonkey");
+    }
+}


Should be more like (for Start and Stop):
+static void
+StartChudRemote()
+{
+    if (gSharkEnabled) {
+        printf("Beginning shark profiling.\n");
+        chudStartRemotePerfMonitor("SpiderMonkey");
+    }
+}

To adhere to surrounding style.  Remove printf()s?

You -could- setup StartChudRemote to take a string parameter, which you then pass to chudStartRemotePerfMonitor(), and let the JS test author control this.  Up to you, I suppose.

Also, should you throw a JS exception in the cases where shark is not enabled?  Would be a nice clue for people wondering what they did wrong.

Comment 7

[Brian Crowder]

Comment on attachment 296203 [details] [diff] [review]
correct ifdef thinko

woops, missed this with last review

Comment 8

[Robert Sayre]

Attachment: Shark everywhere

Put the Shark stuff in jsdbgapi.h per brendan, and enable in-browser.

Comment 9

[Robert Sayre]

> 
> Also, should you throw a JS exception in the cases where shark is not enabled? 
> Would be a nice clue for people wondering what they did wrong.

I did JS_ReportError. An exception turned out to be a pain, because then you have to edit the file if you don't want Shark running momentarily. 

Comment 10

[Brian Crowder]

Comment on attachment 297288 [details] [diff] [review]
Shark everywhere

In this patch, I like even more my suggestion of allowing a JS-provided argument to StartChudRemote that lets JS authors replace your "SpiderMonkey".  If you don't want to do that, it seems better somehow to use "JavaScript" rather than spidermonkey for general purposes and given that this is going into the browser.

Comment 11

[Mike Schroepfer]

yea - me want P1 blocking.

Comment 12

[Robert Sayre]

We'll call this fixed, since people pressed for it today. We can enhance in other bugs.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So one can call the js_* version of the functions from any code that includes jsdbgapi.h, right?

Comment 14

[Robert Sayre]

(In reply to comment #13)
> So one can call the js_* version of the functions from any code that includes
> jsdbgapi.h, right?
> 

That was my aim.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 297288 [details] [diff] [review]
Shark everywhere

>+#ifdef MOZ_SHARK
>+extern JS_FRIEND_API(JSBool) js_StartChudRemote();
>+extern JS_FRIEND_API(JSBool) js_StopChudRemote();
>+extern JS_FRIEND_API(JSBool) js_ConnectShark();
>+extern JS_FRIEND_API(JSBool) js_DisconnectShark();
>+
>+extern JS_FRIEND_API(JSBool) StopShark(JSContext *cx, JSObject *obj,
>+                                       uintN argc, jsval *argv, jsval *rval);
>+
>+extern JS_FRIEND_API(JSBool) StartShark(JSContext *cx, JSObject *obj,
>+                                        uintN argc, jsval *argv, jsval *rval);
>+
>+extern JS_FRIEND_API(JSBool) ConnectShark(JSContext *cx, JSObject *obj,
>+                                          uintN argc, jsval *argv,
>+                                          jsval *rval);
>+
>+extern JS_FRIEND_API(JSBool) DisconnectShark(JSContext *cx, JSObject *obj,
>+                                             uintN argc, jsval *argv,
>+                                             jsval *rval);
>+#endif /* MOZ_SHARK */

Total nit, but could save some renaming on collision later, and it follows the conventions:

* the proper APIs can start JS_, not js_, and for all I care be JS_PUBLIC_APIs;
* the native method entry points have collision-prone names (in a big world program), so could use js_ prefixes (and remain friends);

Question: any reason not to use JSFastNative signature?

/be

Comment 16

[Robert Sayre]

Attachment: renames per brendan

I think this is what you asked for.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 297650 [details] [diff] [review]
renames per brendan

Make the JS_ ones JS_PUBLIC_API (I care more than I let on last time, but you didn't bite ;-) before checkin and no need for more attachments.

/be

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 297650 [details] [diff] [review]
renames per brendan

Looks like you forgot to rename the function names in xpcshell.cpp's glob_functions.

Comment 19

[Robert Sayre]

Attachment: add xpcshell

oops, must have made the previous change in the wrong tree

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 298410 [details] [diff] [review]
add xpcshell

No need for r+sr of this kind, just for future reference. You have my rubber stamp ;-).

/be