RESOLVED FIXED 153638
[GTK][EFL] Enable SamplingProfiler 
https://bugs.webkit.org/show_bug.cgi?id=153638
Summary [GTK][EFL] Enable SamplingProfiler
Yusuke Suzuki
Reported 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
Attachments
Patch (4.64 KB, patch)
2016-02-07 07:31 PST, Yusuke Suzuki 		mcatanzaro: review+
mcatanzaro: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Carlos Alberto Lopez Perez
Comment 1 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.
Yusuke Suzuki
Comment 2 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?
Yusuke Suzuki
Comment 3 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.
Michael Catanzaro
Comment 4 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.
Yusuke Suzuki
Comment 5 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 :)
Yusuke Suzuki
Comment 6 2016-02-07 07:31:37 PST
Created attachment 270816 [details] Patch
Martin Robinson
Comment 7 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?
Michael Catanzaro
Comment 8 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
Yusuke Suzuki
Comment 9 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.
Yusuke Suzuki
Comment 10 2016-02-07 17:34:14 PST
Committed r196245: <http://trac.webkit.org/changeset/196245>
Yusuke Suzuki
Comment 11 2016-02-07 17:36:39 PST
*** Bug 153466 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.