Bug 113144

Summary: JSC_enableProfiler=true should also cause JSGlobalData to save the profiler output somewhere
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cmarcelo, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, ojan.autocc, oliver, rakuco, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch
buildbot: commit-queue-
the patch
buildbot: commit-queue-
the patch
none
the patch ggaren: review+, buildbot: commit-queue-

Description Filip Pizlo 2013-03-23 19:55:51 PDT 
It should save it in the current directory by default, but you should be able to set JSC_PROFILER_PATH to have it save it somewhere else.
Comment 1 Filip Pizlo 2013-03-23 19:59:46 PDT 
Created attachment 194729 [details]
the patch
Comment 2 Filip Pizlo 2013-03-23 20:03:31 PDT 
Created attachment 194730 [details]
the patch
Comment 3 Build Bot 2013-03-23 21:42:52 PDT 
Comment on attachment 194730 [details]
the patch

Attachment 194730 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17139880
Comment 4 Filip Pizlo 2013-03-23 21:45:02 PDT 
Created attachment 194736 [details]
the patch

Spell GetCurrentProcessId correctly this time.
Comment 5 Build Bot 2013-03-23 22:38:46 PDT 
Comment on attachment 194736 [details]
the patch

Attachment 194736 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17139892
Comment 6 Filip Pizlo 2013-03-24 07:42:08 PDT 
Created attachment 194751 [details]
the patch

another Win fix.
Comment 7 Filip Pizlo 2013-03-25 09:27:41 PDT 
Comment on attachment 194751 [details]
the patch

Clearing r? because I need to add some horrible atexit hacks to make this practical. Apparently DRT doesn't delete it's JSGlobalData's.
Comment 8 Geoffrey Garen 2013-03-25 10:37:02 PDT 
> Apparently DRT doesn't delete it's JSGlobalData's.

This is really a WebKit issue: WebCore (intentionally) leaks its singleton JSGlobalData.
Comment 9 Filip Pizlo 2013-03-25 19:08:21 PDT 
Created attachment 194977 [details]
the patch

Made the atexit() thing work.
Comment 10 Geoffrey Garen 2013-03-25 19:36:20 PDT 
Comment on attachment 194977 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194977&action=review

r=me

> Source/JavaScriptCore/profiler/ProfilerDatabase.cpp:37
> +static SpinLock registrationLock;

Needs " = SPINLOCK_INITIALIZER;".
Comment 11 Filip Pizlo 2013-03-25 19:39:03 PDT 
(In reply to comment #10)
> (From update of attachment 194977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194977&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/profiler/ProfilerDatabase.cpp:37
> > +static SpinLock registrationLock;
> 
> Needs " = SPINLOCK_INITIALIZER;".

Will do.

But a question for future reference: isn't it true that on all platforms we care about, globals get initialized to all-zero?  And if that's the case, and SPINLOCK_INITIALIZER is just { 0 }, then can't we just make things simpler and change SpinLock's contract to say that it guarantees that all-zero is a valid unlocked state and that we don't need " = SPINLOCK_INITIALIZER"?
Comment 12 Geoffrey Garen 2013-03-25 21:04:34 PDT 
> But a question for future reference: isn't it true that on all platforms we care about, globals get initialized to all-zero?

Yes. That's a part of the C/C++ ABI.

> And if that's the case, and SPINLOCK_INITIALIZER is just { 0 }

For !ENABLE(COMPARE_AND_SWAP) platforms, that's not the case:

TCSpinLock.h:114: #define SPINLOCK_INITIALIZER { PTHREAD_MUTEX_INITIALIZER }
pthread.h:#define PTHREAD_MUTEX_INITIALIZER {_PTHREAD_MUTEX_SIG_init, {0}}
pthread_impl.h:#define _PTHREAD_MUTEX_SIG_init		0x32AAABA7
Comment 13 Build Bot 2013-03-25 23:32:54 PDT 
Comment on attachment 194977 [details]
the patch

Attachment 194977 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17120904
Comment 14 Filip Pizlo 2013-03-26 14:46:03 PDT 
Landed in http://trac.webkit.org/changeset/146932