Bug 153638 – [GTK][EFL] Enable SamplingProfiler

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Yusuke Suzuki]

We would like to enable Sampling Profiler in GTK and EFL.
Currently, it's usable in GTK and EFL with

- using Pthreads
- using GLIBC
- x64, x86, arm64, arm, mips arch

So we would like to discuss

- what CPU arch GTK and EFL should support
- what standard library GTK and EFL should support

Maybe, I think BSD and Android Bionic are needed. Android Bionic may be used in android-jsc which is used in Facebooks' React Native for Android[1].

[1]: https://github.com/facebook/android-jsc

[Carlos Alberto Lopez Perez]

The GTK+ port currently aims to support x64, x86, ARM, ARM64 and MIPS on Linux with the GNU libc (GLIBC).

Support for running WebKitGTK+ on other architectures or other operating systems / C libraries like BSD, MacOS or Android is something that we don't plan to actively support for the moment. But if there is someone interested on this, and they submit reasonable patches we will try to help with the review.

[Yusuke Suzuki]

(In reply to comment #1)
> The GTK+ port currently aims to support x64, x86, ARM, ARM64 and MIPS on
> Linux with the GNU libc (GLIBC).
> 
> Support for running WebKitGTK+ on other architectures or other operating
> systems / C libraries like BSD, MacOS or Android is something that we don't
> plan to actively support for the moment. But if there is someone interested
> on this, and they submit reasonable patches we will try to help with the
> review.

OK, so now, the current sampling profiler completely meets the GTK port's requirements. So, for GTK port, we would like to make

#if (OS(DARWIN) || OS(WINDOWS) || (USE(PTHREADS) && defined(__GLIBC__))) && ENABLE(JIT)

to

#if (OS(DARWIN) || OS(WINDOWS) || PLATFORM(GTK)) && ENABLE(JIT)

And once sampling profiler is enabled, we will drop legacy profiler.

How about EFL port?

[Yusuke Suzuki]

(In reply to comment #2)
> #if (OS(DARWIN) || OS(WINDOWS) || (USE(PTHREADS) && defined(__GLIBC__))) &&
> ENABLE(JIT)
> 
> to
> 
> #if (OS(DARWIN) || OS(WINDOWS) || PLATFORM(GTK)) && ENABLE(JIT)

This is ENABLE_SAMPLING_PROFILER flag.

[Michael Catanzaro]

I think this needs to be added as a build option in WebKitFeatures.cmake. We would make it public in OptionsGTK.cmake, so it can be disabled on platforms where it does not work.

[Yusuke Suzuki]

(In reply to comment #4)
> I think this needs to be added as a build option in WebKitFeatures.cmake. We
> would make it public in OptionsGTK.cmake, so it can be disabled on platforms
> where it does not work.

Sounds nice :)

[Yusuke Suzuki]

Created attachment 270816 [details]
Patch

[Martin Robinson]

Comment on attachment 270816 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:167
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SAMPLING_PROFILER PUBLIC ON)

I'm not sure this line is necessary. This is a list of private options that differ from the list in WebKitFeatures.cmake. The option you've added is public (shows up in the configuration UI) and the default value is the same. I think you can just remove it. Is there a reason you had in mind to make this option public?

[Michael Catanzaro]

Comment on attachment 270816 [details]
Patch

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

> Source/WTF/wtf/Platform.h:802
>   * sampling profiler is enabled if WebKit uses pthreads and glibc. */

This comment needs to be updated.

>> Source/cmake/OptionsGTK.cmake:167
>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SAMPLING_PROFILER PUBLIC ON)
> 
> I'm not sure this line is necessary. This is a list of private options that differ from the list in WebKitFeatures.cmake. The option you've added is public (shows up in the configuration UI) and the default value is the same. I think you can just remove it. Is there a reason you had in mind to make this option public?

Yeah, it's added in the wrong place in OptionsGTK.cmake, good catch Martin! This should be moved to the list of public options instead. I suggested making it public because it does not work on some platforms, so there should be some way to turn it off.

> Source/cmake/WebKitFeatures.cmake:175
> +    WEBKIT_OPTION_DEFINE(ENABLE_SAMPLING_PROFILER "Toggle sampling profier support" PRIVATE ON)

profier -> profiler

[Yusuke Suzuki]

Comment on attachment 270816 [details]
Patch

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

Thanks for your reviews :)

>>> Source/cmake/OptionsGTK.cmake:167
>>> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SAMPLING_PROFILER PUBLIC ON)
>> 
>> I'm not sure this line is necessary. This is a list of private options that differ from the list in WebKitFeatures.cmake. The option you've added is public (shows up in the configuration UI) and the default value is the same. I think you can just remove it. Is there a reason you had in mind to make this option public?
> 
> Yeah, it's added in the wrong place in OptionsGTK.cmake, good catch Martin! This should be moved to the list of public options instead. I suggested making it public because it does not work on some platforms, so there should be some way to turn it off.

Oops! Thanks :D
I've fixed; moving this flag to PUBLIC list.

>> Source/cmake/WebKitFeatures.cmake:175
>> +    WEBKIT_OPTION_DEFINE(ENABLE_SAMPLING_PROFILER "Toggle sampling profier support" PRIVATE ON)
> 
> profier -> profiler

Thanks! Fixed.

[Yusuke Suzuki]

Committed r196245: <http://trac.webkit.org/changeset/196245>

[Yusuke Suzuki]

*** Bug 153466 has been marked as a duplicate of this bug. ***