Bug 21719 - JS Profiler notification system needed
Summary: JS Profiler notification system needed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Other
: P2 Enhancement
Assignee: Kevin Lindeman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-17 16:13 PDT by Kevin Lindeman
Modified: 2008-10-30 22:25 PDT (History)
3 users (show)

See Also:


Attachments
JSProfileServer proposed patch. (11.36 KB, patch)
2008-10-17 16:19 PDT, Kevin Lindeman
no flags Details | Formatted Diff | Diff
Proposed patch for the JS Profiler notification server. (11.33 KB, patch)
2008-10-17 17:51 PDT, Kevin Lindeman
timothy: review-
Details | Formatted Diff | Diff
3rd patch. (11.31 KB, patch)
2008-10-19 13:24 PDT, Kevin Lindeman
timothy: review-
Details | Formatted Diff | Diff
Patch version 4 (11.32 KB, patch)
2008-10-19 13:49 PDT, Kevin Lindeman
timothy: review-
Details | Formatted Diff | Diff
Patch version 5 (11.30 KB, patch)
2008-10-19 14:27 PDT, Kevin Lindeman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Lindeman 2008-10-17 16:13:56 PDT
A WebKit client needs a way to start and stop the JS Profiler remotely.
Comment 1 Kevin Lindeman 2008-10-17 16:19:33 PDT
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.
Comment 2 Kevin Lindeman 2008-10-17 17:10:37 PDT
I need to change this patch slightly.
Comment 3 Kevin Lindeman 2008-10-17 17:51:11 PDT
Created attachment 24470 [details]
Proposed patch for the JS Profiler notification server.
Comment 4 Timothy Hatcher 2008-10-19 06:36:51 PDT
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>.
Comment 5 Timothy Hatcher 2008-10-19 06:49:46 PDT
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.
Comment 6 Kevin Lindeman 2008-10-19 13:24:11 PDT
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.
Comment 7 Timothy Hatcher 2008-10-19 13:33:02 PDT
(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.
Comment 8 Timothy Hatcher 2008-10-19 13:36:15 PDT
> 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.
Comment 9 Kevin Lindeman 2008-10-19 13:49:00 PDT
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?
Comment 10 Timothy Hatcher 2008-10-19 14:19:19 PDT
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());
+}
Comment 11 Kevin Lindeman 2008-10-19 14:27:07 PDT
Created attachment 24514 [details]
Patch version 5
Comment 12 Timothy Hatcher 2008-10-30 22:25:13 PDT
Landed in r38022.