Add JS function to start and stop Shark programatically.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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".
(Assignee)

Comment 1

9 years ago
Created attachment 296039 [details] [diff] [review]
add Shark functions to js.c

I should add that I cribbed the CHUD incantations from a similar patch that bz wrote for nsJSEnvironment.
Assignee: general → sayrer
Status: NEW → ASSIGNED
Attachment #296039 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #296039 - Flags: review? → review?(brendan)
(Assignee)

Comment 2

9 years ago
Created attachment 296040 [details]
screenshot of fully-expanded bitwise-and profile

Comment 3

9 years ago
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.

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

See comment 0, the "This feature requires" part.
(Assignee)

Comment 5

9 years ago
Created attachment 296203 [details] [diff] [review]
correct ifdef thinko
Attachment #296039 - Attachment is obsolete: true
Attachment #296203 - Flags: review?
Attachment #296039 - Flags: review?(brendan)
(Assignee)

Updated

9 years ago
Attachment #296203 - Flags: review? → review?(crowder)

Comment 6

9 years ago
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

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

woops, missed this with last review
Attachment #296203 - Flags: review?(crowder) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 297288 [details] [diff] [review]
Shark everywhere

Put the Shark stuff in jsdbgapi.h per brendan, and enable in-browser.
Attachment #296203 - Attachment is obsolete: true
Attachment #297288 - Flags: superreview?
Attachment #297288 - Flags: review?(crowder)
(Assignee)

Updated

9 years ago
Attachment #297288 - Flags: superreview? → superreview?(jst)
(Assignee)

Updated

9 years ago
Attachment #297288 - Attachment is patch: true
Attachment #297288 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 9

9 years ago
> 
> 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

9 years ago
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.

Updated

9 years ago
Attachment #297288 - Flags: review?(crowder) → review+

Comment 11

9 years ago
yea - me want P1 blocking.
Flags: blocking1.9+
Priority: -- → P1

Updated

9 years ago
Attachment #297288 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 12

9 years ago
We'll call this fixed, since people pressed for it today. We can enhance in other bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
So one can call the js_* version of the functions from any code that includes jsdbgapi.h, right?
(Assignee)

Comment 14

9 years ago
(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 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
(Assignee)

Comment 16

9 years ago
Created attachment 297650 [details] [diff] [review]
renames per brendan

I think this is what you asked for.
Attachment #297650 - Flags: review?(brendan)
(Assignee)

Updated

9 years ago
Depends on: 412874
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
Attachment #297650 - Flags: review?(brendan)
Attachment #297650 - Flags: review+
Attachment #297650 - Flags: approval1.9+
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.
(Assignee)

Comment 19

9 years ago
Created attachment 298410 [details] [diff] [review]
add xpcshell

oops, must have made the previous change in the wrong tree
Attachment #297650 - Attachment is obsolete: true
Attachment #298410 - Flags: superreview?(brendan)
Attachment #298410 - Flags: review?(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
Attachment #298410 - Flags: superreview?(brendan)
Attachment #298410 - Flags: superreview+
Attachment #298410 - Flags: review?(brendan)
Attachment #298410 - Flags: review+

Updated

9 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.