Bug 141781 – ChildProcess should take an os_activity

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Gavin Barraclough]

This should make activity tracing work.

[Gavin Barraclough]

Created attachment 246860 [details]
Fix

[Anders Carlsson]

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?

[Ryosuke Niwa]

Created attachment 247581 [details]
Cleanup; Fixed the build

[Ryosuke Niwa]

<rdar://problem/19818204>

[Sam Weinig]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

I guess I should be calling os_activity_end inside ChildProcess::terminate instead?

[Sam Weinig]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 247695 [details]
Got rid of macro

[David Kilzer (:ddkilzer)]

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.

[Ryosuke Niwa]

Created attachment 247707 [details]
Fixed Mavericks build

[Sam Weinig]

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.

[David Kilzer (:ddkilzer)]

Comment on attachment 247707 [details]
Fixed Mavericks build

r- per Sam's Comment #14.

[Sam Weinig]

Created attachment 252916 [details]
Patch

[Sam Weinig]

Created attachment 252922 [details]
Patch

[Ryosuke Niwa]

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?

[Sam Weinig]

Committed revision 184203.

[Sam Weinig]

(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 :).