Bug 21719 – JS Profiler notification system needed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kevin Lindeman]

A WebKit client needs a way to start and stop the JS Profiler remotely.

[Kevin Lindeman]

Created attachment 24467 [details]
JSProfileServer proposed patch.

Timothy Hatcher has taken a look at this already, but here it is for someone else to review as well.

[Kevin Lindeman]

I need to change this patch slightly.

[Kevin Lindeman]

Created attachment 24470 [details]
Proposed patch for the JS Profiler notification server.

[Timothy Hatcher]

Comment on attachment 24470 [details]
Proposed patch for the JS Profiler notification server.

+#import <Foundation/NSDistributedNotificationCenter.h>
+#import <Foundation/NSProcessInfo.h>
+#import <Foundation/NSString.h>
+#import <Foundation/NSUserDefaults.h>
+#import "JSStringRef.h"

All of these #imports should move to the .mm file, you only need to #import NSObject.h in the JSProfileServer.h header. And you should add @class NSString to the header.

You need a header guard around the contents of JSProfileServer.h.

#ifndef JSProfileServer_h
#define JSProfileServer_h

// ...

#endif // JSProfileServer_h

I am thinking you don't even need to put the ObjC @interface in the header, since no one in the process needs to call it except in the .mm file. So just put startSharedProfileServer() in the header. Then you can remove the #ifdef __OBJC__ and #ifdef __cplusplus. The header will be C++ only, and the .mm with be ObjC++.

Also startSharedProfileServer() should be put in the JSC C++ namespace.

+static JSProfileServer *sharedServer = nil;

That can move into the sharedProfileServer class method, since only it accesses it. Also you can leave off the "= nil" part, since statics default to 0.

+    listenerCount++;
+    if (listenerCount > 0) {
+        JSStringRef profileName = JSStringCreateWithUTF8CString([serverName UTF8String]);
+        JSStartProfiling(0, profileName);
+        JSStringRelease(profileName);
+    }

I think that should be:

+    if (++listenerCount > 1)
+        return;
+    JSRetainPtr<JSStringRef> profileName(Adopt, JSStringCreateWithUTF8CString([serverName UTF8String]));
+    JSStartProfiling(0, profileName.get());

I don't think you want to start the profiler multiple times if another start is requested (since you don't end it the same number of times in stopProfiling). Also you can use JSRetainPtr since this is C++, and that will do the JSStringRelease for you. Just include JSRetainPtr.h.

+    listenerCount--;
+    if (listenerCount <= 0) {

That can be an early return.

+    ASSERT(listenerCount);
+    if (--listenerCount > 0)
+        return;
+    JSRetainPtr<JSStringRef> profileName(Adopt, JSStringCreateWithUTF8CString([serverName UTF8String]));
+    JSEndProfiling(0, profileName.get());

Also you should assert the listenerCount is non-zero before decrementing it. Include <wtf/Assertions.h>.

[Timothy Hatcher]

Comment on attachment 24470 [details]
Proposed patch for the JS Profiler notification server.

+NSString *JSProfileServerStartNotification = @"JSProfileServerStartNotification";
+NSString *JSProfileServerStopNotification = @"JSProfileServerStopNotification";

Those strings don't need to be global, they should just be variables in init.

[Kevin Lindeman]

Created attachment 24511 [details]
3rd patch.

Here is the patch with your requested changes. I didn't do an assertion check that listenerCount > 0 in the stop method (like I think you suggested), since it is possible that we use the global stop notification, after a new app using webkit is launched, at which point the listenerCount on that one is 0.

[Timothy Hatcher]

(In reply to comment #6)
> Here is the patch with your requested changes. I didn't do an assertion check
> that listenerCount > 0 in the stop method (like I think you suggested), since
> it is possible that we use the global stop notification, after a new app using
> webkit is launched, at which point the listenerCount on that one is 0.

That is a good point. The patch has the assert though.

More importantly, you need to return early if listenerCount == 0, otherwise listenerCount will underflow since it is unsigned.

[Timothy Hatcher]

> More importantly, you need to return early if listenerCount == 0, otherwise
> listenerCount will underflow since it is unsigned.

Our style guideline say use !listenerCount, so you want to do that instead of "listenerCount == 0" like I said.

[Kevin Lindeman]

Created attachment 24512 [details]
Patch version 4

I checked to make sure this worked in the "launch after we start" case mentioned and it does.

The only thing that I wonder about with this patch is the if (!--listenerCount) after we check to make sure we aren't 0 first - it works correctly, but looks ugly. Any suggestions?

[Timothy Hatcher]

Comment on attachment 24512 [details]
Patch version 4

You didn't need to remove the other early return, that would make the code less ugly.

This is what you want:

+- (void)stopProfiling
+{
+    if (!listenerCount || --listenerCount > 0)
+        return;
+    JSRetainPtr<JSStringRef> profileName(Adopt, JSStringCreateWithUTF8CString([serverName UTF8String]));
+    JSEndProfiling(0, profileName.get());
+}

[Kevin Lindeman]

Created attachment 24514 [details]
Patch version 5

[Timothy Hatcher]

Landed in r38022.