This should make activity tracing work.
Created attachment 246860 [details] Fix
Comment on attachment 246860 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246860&action=review > Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm:64 > + m_osActivity = os_activity_start("com.apple.WebKit.Networking", OS_ACTIVITY_FLAG_DEFAULT); Why do you need to do this in initializeProcessName, as opposed to platformInitialize?
Created attachment 247581 [details] Cleanup; Fixed the build
<rdar://problem/19818204>
Comment on attachment 247581 [details] Cleanup; Fixed the build View in context: https://bugs.webkit.org/attachment.cgi?id=247581&action=review > Source/WebKit2/Shared/ChildProcess.cpp:42 > +#if HAVE(OS_ACTIVITY) > + m_osActivity = OS_ACTIVITY_NULL; > +#endif Can we do this with an in header initializer? > Source/WebKit2/Shared/ChildProcess.cpp:50 > +#if HAVE(OS_ACTIVITY) > + if (m_osActivity != OS_ACTIVITY_NULL) > + os_activity_end(m_osActivity); > +#endif When will this actually ever be called? I think all the ChildProcess instances are actually singletons. Is it important for this to run? > Source/WebKit2/Shared/ChildProcess.h:106 > +#if HAVE(OS_ACTIVITY) > + // We can't make this private because os_activity_start forces __builtin_constant_p on name. > + os_activity_t m_osActivity; > +#define startOSActivity(name) { m_osActivity = os_activity_start(name, OS_ACTIVITY_FLAG_DEFAULT); }; > +#else > +#define startOSActivity(name) { }; > +#endif This seems a bit gross to me. I have two ideas about how to make it a bit nicer: 1) Get rid of the macro. Just call os_activity_start(name, OS_ACTIVITY_FLAG_DEFAULT); directly with some #ifdefs in the few places where it needs to be called. 2) Move all this activity code to process entry points (see WebContentServiceInitializer.mm, DatabaseServiceInitializer.mm etc.) We might even be able to make use of the XPCServiceInitializer helper with some static const names.
(In reply to comment #5) > Comment on attachment 247581 [details] > Cleanup; Fixed the build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247581&action=review > > > Source/WebKit2/Shared/ChildProcess.cpp:42 > > +#if HAVE(OS_ACTIVITY) > > + m_osActivity = OS_ACTIVITY_NULL; > > +#endif > > Can we do this with an in header initializer? If we did that, we need to either move the member variable to somewhere below the first private member variable is defined or if-def-else the initialization list because the first initializations needs to be prefixed with ':' and not ','. i.e. ChildProcess::ChildProcess() #if HAVE(OS_ACTIVITY) : m_osActivity(OS_ACTIVITY_NULL) , m_terminationTimeout(0) #else : m_terminationTimeout(0) #endif > > Source/WebKit2/Shared/ChildProcess.cpp:50 > > +#if HAVE(OS_ACTIVITY) > > + if (m_osActivity != OS_ACTIVITY_NULL) > > + os_activity_end(m_osActivity); > > +#endif > > When will this actually ever be called? I think all the ChildProcess > instances are actually singletons. Is it important for this to run? We need to run this code when the child process terminates. > > Source/WebKit2/Shared/ChildProcess.h:106 > > +#if HAVE(OS_ACTIVITY) > > + // We can't make this private because os_activity_start forces __builtin_constant_p on name. > > + os_activity_t m_osActivity; > > +#define startOSActivity(name) { m_osActivity = os_activity_start(name, OS_ACTIVITY_FLAG_DEFAULT); }; > > +#else > > +#define startOSActivity(name) { }; > > +#endif > > This seems a bit gross to me. I have two ideas about how to make it a bit > nicer: > > 1) Get rid of the macro. Just call os_activity_start(name, > OS_ACTIVITY_FLAG_DEFAULT); directly with some #ifdefs in the few places > where it needs to be called. That's what Gavin's original patch did. I didn't want to repeat OS_ACTIVITY_FLAG_DEFAULT everywhere but perhaps that's less gross... > 2) Move all this activity code to process entry points (see > WebContentServiceInitializer.mm, DatabaseServiceInitializer.mm etc.) We > might even be able to make use of the XPCServiceInitializer helper with some > static const names. I don't think that'll work because of __builtin_constant_p. Basically, os_activity_start needs to be "called" with a string literal.
I guess I should be calling os_activity_end inside ChildProcess::terminate instead?
(In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 247581 [details] > > Cleanup; Fixed the build > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=247581&action=review > > > > > Source/WebKit2/Shared/ChildProcess.cpp:42 > > > +#if HAVE(OS_ACTIVITY) > > > + m_osActivity = OS_ACTIVITY_NULL; > > > +#endif > > > > Can we do this with an in header initializer? > > If we did that, we need to either move the member variable to somewhere > below the first private member variable is defined or if-def-else the > initialization list because the first initializations needs to be prefixed > with ':' and not ','. i.e. > > ChildProcess::ChildProcess() > #if HAVE(OS_ACTIVITY) > : m_osActivity(OS_ACTIVITY_NULL) > , m_terminationTimeout(0) > #else > : m_terminationTimeout(0) > #endif > > > > Source/WebKit2/Shared/ChildProcess.cpp:50 > > > +#if HAVE(OS_ACTIVITY) > > > + if (m_osActivity != OS_ACTIVITY_NULL) > > > + os_activity_end(m_osActivity); > > > +#endif > > > > When will this actually ever be called? I think all the ChildProcess > > instances are actually singletons. Is it important for this to run? > > We need to run this code when the child process terminates. Why? What does it do? What happens if we don't? > > > > Source/WebKit2/Shared/ChildProcess.h:106 > > > +#if HAVE(OS_ACTIVITY) > > > + // We can't make this private because os_activity_start forces __builtin_constant_p on name. > > > + os_activity_t m_osActivity; > > > +#define startOSActivity(name) { m_osActivity = os_activity_start(name, OS_ACTIVITY_FLAG_DEFAULT); }; > > > +#else > > > +#define startOSActivity(name) { }; > > > +#endif > > > > This seems a bit gross to me. I have two ideas about how to make it a bit > > nicer: > > > > 1) Get rid of the macro. Just call os_activity_start(name, > > OS_ACTIVITY_FLAG_DEFAULT); directly with some #ifdefs in the few places > > where it needs to be called. > > That's what Gavin's original patch did. I didn't want to repeat > OS_ACTIVITY_FLAG_DEFAULT everywhere but perhaps that's less gross... I think it is quite a bit less gross than a public member. > > > 2) Move all this activity code to process entry points (see > > WebContentServiceInitializer.mm, DatabaseServiceInitializer.mm etc.) We > > might even be able to make use of the XPCServiceInitializer helper with some > > static const names. > > I don't think that'll work because of __builtin_constant_p. Basically, > os_activity_start needs to be "called" with a string literal. Well, with the magic of templates, you should be able to provide a real compile time literal.
(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #5) > > > Comment on attachment 247581 [details] > > > Cleanup; Fixed the build > > > 2) Move all this activity code to process entry points (see > > > WebContentServiceInitializer.mm, DatabaseServiceInitializer.mm etc.) We > > > might even be able to make use of the XPCServiceInitializer helper with some > > > static const names. > > > > I don't think that'll work because of __builtin_constant_p. Basically, > > os_activity_start needs to be "called" with a string literal. > > Well, with the magic of templates, you should be able to provide a real > compile time literal. No. It needs to be a string literal at the pre-processor time because os_activity_start is a macro, not a function.
(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #5) > > > Comment on attachment 247581 [details] > > > Cleanup; Fixed the build > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=247581&action=review > > > > > > > Source/WebKit2/Shared/ChildProcess.cpp:42 > > > > +#if HAVE(OS_ACTIVITY) > > > > + m_osActivity = OS_ACTIVITY_NULL; > > > > +#endif > > > > > > Can we do this with an in header initializer? > > > > If we did that, we need to either move the member variable to somewhere > > below the first private member variable is defined or if-def-else the > > initialization list because the first initializations needs to be prefixed > > with ':' and not ','. i.e. > > > > ChildProcess::ChildProcess() > > #if HAVE(OS_ACTIVITY) > > : m_osActivity(OS_ACTIVITY_NULL) > > , m_terminationTimeout(0) > > #else > > : m_terminationTimeout(0) > > #endif > > > > > > Source/WebKit2/Shared/ChildProcess.cpp:50 > > > > +#if HAVE(OS_ACTIVITY) > > > > + if (m_osActivity != OS_ACTIVITY_NULL) > > > > + os_activity_end(m_osActivity); > > > > +#endif > > > > > > When will this actually ever be called? I think all the ChildProcess > > > instances are actually singletons. Is it important for this to run? > > > > We need to run this code when the child process terminates. > > Why? What does it do? What happens if we don't? I don't know.
Created attachment 247695 [details] Got rid of macro
Comment on attachment 247695 [details] Got rid of macro View in context: https://bugs.webkit.org/attachment.cgi?id=247695&action=review > Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:46 > + m_osActivity = os_activity_start("com.apple.WebKit.Databases", OS_ACTIVITY_FLAG_DEFAULT); Need #if HAVE(OS_ACTIVITY)/#endif around this line in each file.
Created attachment 247707 [details] Fixed Mavericks build
Comment on attachment 247707 [details] Fixed Mavericks build Given that these never really need to be destroyed, I think putting them WebContentServiceInitializer (WebContentServiceEntryPoint.mm) and related places is the right way to go. That will keep it in platform specific code and not litter the cross platform FooProcess classes.
Comment on attachment 247707 [details] Fixed Mavericks build r- per Sam's Comment #14.
Created attachment 252916 [details] Patch
Created attachment 252922 [details] Patch
Comment on attachment 252922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252922&action=review > Source/WebKit2/DatabaseProcess/EntryPoint/mac/XPCService/DatabaseServiceEntryPoint.mm:51 > +#if HAVE(OS_ACTIVITY) > + os_activity_end(activity); > +#endif I guess this code never runs?
Committed revision 184203.
(In reply to comment #18) > Comment on attachment 252922 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252922&action=review > > > Source/WebKit2/DatabaseProcess/EntryPoint/mac/XPCService/DatabaseServiceEntryPoint.mm:51 > > +#if HAVE(OS_ACTIVITY) > > + os_activity_end(activity); > > +#endif > > I guess this code never runs? In most cases no, but it makes the compiler happy :).