Bug 153638 - [GTK][EFL] Enable SamplingProfiler
Summary: [GTK][EFL] Enable SamplingProfiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
: 153466 (view as bug list)
Depends on: 154033
Blocks: 153466
  Show dependency treegraph
 
Reported: 2016-01-28 21:28 PST by Yusuke Suzuki
Modified: 2016-02-09 09:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.64 KB, patch)
2016-02-07 07:31 PST, Yusuke Suzuki
mcatanzaro: review+
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-01-28 21:28:57 PST
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
Comment 1 Carlos Alberto Lopez Perez 2016-02-02 04:54:15 PST
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.
Comment 2 Yusuke Suzuki 2016-02-02 06:27:30 PST
(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?
Comment 3 Yusuke Suzuki 2016-02-02 06:28:14 PST
(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.
Comment 4 Michael Catanzaro 2016-02-02 10:30:23 PST
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.
Comment 5 Yusuke Suzuki 2016-02-02 11:24:28 PST
(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 :)
Comment 6 Yusuke Suzuki 2016-02-07 07:31:37 PST
Created attachment 270816 [details]
Patch
Comment 7 Martin Robinson 2016-02-07 13:34:29 PST
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?
Comment 8 Michael Catanzaro 2016-02-07 14:26:27 PST
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
Comment 9 Yusuke Suzuki 2016-02-07 17:31:01 PST
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.
Comment 10 Yusuke Suzuki 2016-02-07 17:34:14 PST
Committed r196245: <http://trac.webkit.org/changeset/196245>
Comment 11 Yusuke Suzuki 2016-02-07 17:36:39 PST
*** Bug 153466 has been marked as a duplicate of this bug. ***