Bug 140162

Summary: [EFL][GTK] Use bmalloc instead of tcmalloc
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, cdumez, cgarcia, changseok, clopez, commit-queue, ddkilzer, ggaren, hyuki.kim, kvserr, laszlo.gombos, lucas.de.marchi, mcatanzaro, mrobinson, olivier.blin, ossy, pnormand, sergio, ysuzuki, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 141472    
Bug Blocks:    
Attachments:
Description Flags
Very very WIP
none
WIP-2
none
Additional changes
none
WIP-3 with Zan's change
none
WIP-4
none
WIP-5
none
WIP-6
none
Patch
none
Patch
none
Patch for ews
none
Patch for ews
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Gyuyoung Kim
Reported 2015-01-06 18:29:12 PST
As announced in webkit-dev - https://lists.webkit.org/pipermail/webkit-dev/2015-January/027135.html, WebKit EFL needs to consider to use bmalloc instead of TCMalloc.
Attachments
Very very WIP (3.33 KB, patch)
2015-01-09 07:52 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
WIP-2 (6.17 KB, patch)
2015-01-13 08:32 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Additional changes (1.60 KB, patch)
2015-01-17 07:08 PST, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
WIP-3 with Zan's change (8.44 KB, patch)
2015-01-17 08:10 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
WIP-4 (12.72 KB, patch)
2015-01-23 22:05 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
WIP-5 (14.45 KB, patch)
2015-01-24 23:27 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
WIP-6 (13.57 KB, patch)
2015-01-25 05:19 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (14.18 KB, patch)
2015-01-25 07:19 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (15.87 KB, patch)
2015-01-25 18:08 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch for ews (15.64 KB, patch)
2015-01-26 09:11 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch for ews (15.60 KB, patch)
2015-01-26 17:27 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (17.11 KB, patch)
2015-01-27 08:34 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (18.04 KB, patch)
2015-01-29 00:31 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (18.00 KB, patch)
2015-01-29 21:17 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (17.99 KB, patch)
2015-01-30 17:15 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (16.86 KB, patch)
2015-01-30 19:19 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (17.00 KB, patch)
2015-02-01 23:17 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (17.13 KB, patch)
2015-02-02 05:04 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (16.96 KB, patch)
2015-02-03 17:06 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (17.20 KB, patch)
2015-02-09 04:12 PST, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Show Obsolete (19) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2015-01-07 11:53:51 PST
That's great to hear! Just FYI, I tested bmalloc on Linux[1] with small patches[2] before and it worked very fine. [1]: https://github.com/Constellation/bmalloc-ported [2]: https://github.com/Constellation/bmalloc-ported/tree/master/tools
Gyuyoung Kim
Comment 2 2015-01-09 07:52:50 PST
Created attachment 244340 [details] Very very WIP
Carlos Garcia Campos
Comment 3 2015-01-12 03:43:45 PST
It seems the changes needed will be common to GTK and EFL port, so maybe we should remove the EFL prefix from the bug or add GTK there.
Gyuyoung Kim
Comment 4 2015-01-12 03:55:27 PST
(In reply to comment #3) > It seems the changes needed will be common to GTK and EFL port, so maybe we > should remove the EFL prefix from the bug or add GTK there. That's right. And sorry for late uploading patch. Nowadays I was busy. Let me try to upload new patch.
Gyuyoung Kim
Comment 5 2015-01-13 08:32:39 PST
Created attachment 244516 [details] WIP-2
Zan Dobersek
Comment 6 2015-01-17 07:08:35 PST
Created attachment 244839 [details] Additional changes I made some additional changes on top of the latest patch to get bmalloc building and properly linked. Mimicking Source/WTF/wtf/VMTags.h, BMALLOC_VM_TAG should be defined to -1. We don't have MADV_FREE_REUSABLE or MADV_FREE_REUSE on Linux, so likely MADV_NORMAL should be used instead. Performance improvement is nice. http://people.igalia.com/zdobersek/bmalloc/PerformanceTestsResults.html
Gyuyoung Kim
Comment 7 2015-01-17 08:10:52 PST
Created attachment 244840 [details] WIP-3 with Zan's change
Gyuyoung Kim
Comment 8 2015-01-17 08:13:17 PST
(In reply to comment #6) > Created attachment 244839 [details] > Additional changes > > I made some additional changes on top of the latest patch to get bmalloc > building and properly linked. > > Mimicking Source/WTF/wtf/VMTags.h, BMALLOC_VM_TAG should be defined to -1. > We don't have MADV_FREE_REUSABLE or MADV_FREE_REUSE on Linux, so likely > MADV_NORMAL should be used instead. > > Performance improvement is nice. > http://people.igalia.com/zdobersek/bmalloc/PerformanceTestsResults.html Thank you Zan ! I merge your change with my new small update. Besides the performance result is impressive.
Zan Dobersek
Comment 9 2015-01-17 08:42:40 PST
Comment on attachment 244840 [details] WIP-3 with Zan's change View in context: https://bugs.webkit.org/attachment.cgi?id=244840&action=review > Source/bmalloc/bmalloc/VMAllocate.h:117 > - SYSCALL(madvise(p, vmSize, MADV_FREE_REUSABLE)); > + SYSCALL(madvise(p, vmSize, MADV_NORMAL)); This should of course stay MADV_FREE_REUSABLE if defined(BOS_DARWIN). > Source/bmalloc/bmalloc/VMAllocate.h:123 > - SYSCALL(madvise(p, vmSize, MADV_FREE_REUSE)); > + SYSCALL(madvise(p, vmSize, MADV_NORMAL)); And the same for MADV_FREE_REUSE.
WebKit Commit Bot
Comment 10 2015-01-19 04:40:57 PST
Attachment 244840 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/Vector.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Geoffrey Garen
Comment 11 2015-01-19 11:35:58 PST
Comment on attachment 244840 [details] WIP-3 with Zan's change View in context: https://bugs.webkit.org/attachment.cgi?id=244840&action=review >> Source/bmalloc/bmalloc/VMAllocate.h:117 >> + SYSCALL(madvise(p, vmSize, MADV_NORMAL)); > > This should of course stay MADV_FREE_REUSABLE if defined(BOS_DARWIN). NORMAL is wrong for deallocation; you want MADV_FREE or MADV_DONTNEED. In order to make this work on Windows, we'll need to abstract out mmap, munmap, madvise, and getpagesize. Perhaps we should start now, but creating VMAllocateDarwin.h and VMAllocateLinux.h, and putting platform-specific code in them, namely: vmTruePageSize() -- getpagesize vmAllocate(size_t vmSize) -- mmap vmDeallocate(void* p, size_t vmSize) -- munmap vmDeallocatePhysicalPages(void* p, size_t vmSize) -- madvise vmAllocatePhysicalPages(void* p, size_t vmSize) -- madvise I like putting platform abstractions like this into separate files because I find lots of #ifdefs hard to read.
Csaba Osztrogonác
Comment 12 2015-01-22 07:47:12 PST
Comment on attachment 244840 [details] WIP-3 with Zan's change View in context: https://bugs.webkit.org/attachment.cgi?id=244840&action=review > Source/cmake/WebKitHelpers.cmake:35 > + set(OLD_COMPILE_FLAGS "-Werror -Wno-error=attributes -Wno-error=unused-parameter -Wno-error=uninitialized -Wno-error=literal-suffix ${OLD_COMPILE_FLAGS}") Can't we fix these warnings instead of mute them everywhere?
Gyuyoung Kim
Comment 13 2015-01-23 22:05:38 PST
Created attachment 245276 [details] WIP-4
Gyuyoung Kim
Comment 14 2015-01-23 22:12:02 PST
(In reply to comment #12) > Comment on attachment 244840 [details] > WIP-3 with Zan's change > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244840&action=review > > > Source/cmake/WebKitHelpers.cmake:35 > > + set(OLD_COMPILE_FLAGS "-Werror -Wno-error=attributes -Wno-error=unused-parameter -Wno-error=uninitialized -Wno-error=literal-suffix ${OLD_COMPILE_FLAGS}") > > Can't we fix these warnings instead of mute them everywhere? I will remove all warning caused by using bmalloc. However I would have working version to do fix all warning more easily at the moment. BTW there are many build errors caused by "typedef typename Traits::Page Page" as below, In file included from ../../Source/bmalloc/bmalloc/MediumLine.h:30:0, from ../../Source/bmalloc/bmalloc/MediumChunk.h:30, from ../../Source/bmalloc/bmalloc/Heap.h:32, from ../../Source/bmalloc/bmalloc/Deallocator.cpp:30: ../../Source/bmalloc/bmalloc/MediumTraits.h:43:33: error: declaration of ‘typedef class bmalloc::Chunk<bmalloc::MediumTraits> bmalloc::MediumTraits::Chunk’ [-fpermissive] typedef Chunk<MediumTraits> Chunk; ^ To fix this, I change "Traits::Page" with "Traits::PageTraits" using #ifdef PLATFORM(EFL) || PLATFORM(GTK) for now. However, as you know, this causes #ifdef hell in our code. Does anyone know how to fix this problem using more clear method ?
David Kilzer (:ddkilzer)
Comment 15 2015-01-23 23:02:52 PST
I don't think bmalloc should depend on WTF: /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/VMAllocate.h:36:10: fatal error: 'wtf/Platform.h' file not found #include <wtf/Platform.h> ^ 1 error generated. It will need its own concept of platform macros as it is intended to stand alone (not depend on WTF headers or code).
Gyuyoung Kim
Comment 16 2015-01-24 23:27:00 PST
Created attachment 245294 [details] WIP-5
Gyuyoung Kim
Comment 17 2015-01-24 23:27:44 PST
(In reply to comment #15) > I don't think bmalloc should depend on WTF: > > /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/VMAllocate.h:36:10: fatal > error: 'wtf/Platform.h' file not found > #include <wtf/Platform.h> > ^ > 1 error generated. > > It will need its own concept of platform macros as it is intended to stand > alone (not depend on WTF headers or code). Ok, I remove the WTF dependency from bmalloc.
Gyuyoung Kim
Comment 18 2015-01-25 05:19:39 PST
Created attachment 245305 [details] WIP-6
Gyuyoung Kim
Comment 19 2015-01-25 07:19:55 PST
Created attachment 245306 [details] Patch
Zan Dobersek
Comment 20 2015-01-25 08:47:28 PST
Comment on attachment 245306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245306&action=review > Source/bmalloc/bmalloc/AsyncTask.h:69 > +#if !defined(BUILDING_EFL__) && !defined(BUILDING_GTK__) > template<typename Object, typename Function> const std::chrono::seconds AsyncTask<Object, Function>::exitDelay; > +#endif Add the constexpr specifier, like in the declaration, and it should build fine. > Source/bmalloc/bmalloc/BoundaryTag.h:32 > +#include <memory.h> memcpy/memset are provided through string.h. Better yet, include <cstring> and change memcpy/memset to std::memcpy/std::memset. > Source/bmalloc/bmalloc/MediumTraits.h:41 > - typedef Chunk<MediumTraits> Chunk; > - typedef Line<MediumTraits> Line; > - typedef Page<MediumTraits> Page; > + typedef Chunk<MediumTraits> ChunkTraits; > + typedef Line<MediumTraits> LineTraits; > + typedef Page<MediumTraits> PageTraits; I think ChunkType, LineType and PageType would fit better than ChunkTraits, LineTraits and PageTraits. > Source/bmalloc/bmalloc/PerThread.h:56 > -#if defined(__has_include) && __has_include(<System/pthread_machdep.h>) > +#if defined(__has_include) > +#if __has_include(<System/pthread_machdep.h>) Hypothetically, if a compiler didn't define __has_include, you'd end up missing code. GCC 4.8 and 4.9 support this. Not sure about 4.7 and MSVC. Anyway, you could check for <System/pthread_machdep.h> this way only once, define e.g. HAS_PTHREAD_MACHDEP_H accordingly, and then test this macro in both places. > Source/bmalloc/bmalloc/SmallTraits.h:40 > - typedef Chunk<SmallTraits> Chunk; > - typedef Line<SmallTraits> Line; > - typedef Page<SmallTraits> Page; > + typedef Chunk<SmallTraits> ChunkTraits; > + typedef Line<SmallTraits> LineTraits; > + typedef Page<SmallTraits> PageTraits; I think ChunkType, LineType and PageType would fit better than ChunkTraits, LineTraits and PageTraits. > Source/bmalloc/bmalloc/VMAllocate.h:121 > +#else > + SYSCALL(madvise(p, vmSize, MADV_NORMAL)); > +#endif As per comment #11, this should either be MADV_FREE or MADV_DONTNEED. > Source/bmalloc/bmalloc/Vector.h:32 > +#include <memory.h> memcpy/memset are provided through string.h. Better yet, include <cstring> and change memcpy/memset to std::memcpy/std::memset.
Gyuyoung Kim
Comment 21 2015-01-25 18:08:15 PST
Created attachment 245321 [details] Patch
Gyuyoung Kim
Comment 22 2015-01-25 18:13:29 PST
Comment on attachment 245306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245306&action=review Otherwise done. >> Source/bmalloc/bmalloc/AsyncTask.h:69 >> +#endif > > Add the constexpr specifier, like in the declaration, and it should build fine. I will try to do this when I'm not busy. BTW, could you suggest an example for this case ? >> Source/bmalloc/bmalloc/PerThread.h:56 >> +#if __has_include(<System/pthread_machdep.h>) > > Hypothetically, if a compiler didn't define __has_include, you'd end up missing code. > GCC 4.8 and 4.9 support this. Not sure about 4.7 and MSVC. > > Anyway, you could check for <System/pthread_machdep.h> this way only once, define e.g. HAS_PTHREAD_MACHDEP_H accordingly, and then test this macro in both places. Yes, this is a problem. Let me try to use your suggestion soon.
WebKit Commit Bot
Comment 23 2015-01-25 18:16:22 PST
Attachment 245321 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/PerThread.h:58: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/bmalloc/bmalloc/PerThread.h:58: Missing spaces around / [whitespace/operators] [3] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zan Dobersek
Comment 24 2015-01-26 01:12:14 PST
Comment on attachment 245306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245306&action=review >>> Source/bmalloc/bmalloc/AsyncTask.h:69 >>> +#endif >> >> Add the constexpr specifier, like in the declaration, and it should build fine. > > I will try to do this when I'm not busy. BTW, could you suggest an example for this case ? Just add the constexpr specifier, to mach the declaration: template<typename Object, typename Function> const constexpr std::chrono::seconds AsyncTask<Object, Function>::exitDelay;
Gyuyoung Kim
Comment 25 2015-01-26 09:11:20 PST
Created attachment 245352 [details] Patch for ews
Geoffrey Garen
Comment 26 2015-01-26 11:16:53 PST
Comment on attachment 245352 [details] Patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=245352&action=review > Source/bmalloc/bmalloc/VMAllocate.h:37 > +#if defined(BOS_DARWIN) The correct way to write this is "BOS(DARWIN)".
Gyuyoung Kim
Comment 27 2015-01-26 17:27:39 PST
Created attachment 245395 [details] Patch for ews
Gyuyoung Kim
Comment 28 2015-01-27 08:34:54 PST
Created attachment 245447 [details] Patch
Gyuyoung Kim
Comment 29 2015-01-27 08:38:17 PST
Though this patch may have still problems, it seems to me that this patch is ready to get official review.
Geoffrey Garen
Comment 30 2015-01-27 11:25:01 PST
Comment on attachment 245447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245447&action=review Looks like the GTK build failed. More comments below. > Source/bmalloc/bmalloc/PerThread.h:57 > +#if defined(__has_include) > +#if __has_include(<System/pthread_machdep.h>) Why did this have to change? > Source/bmalloc/bmalloc/PerThread.h:82 > +#if defined(__has_include) || defined(BUILDING_EFL__) This is wrong. To match the conditions above, it would need to read "#if !defined(__has_include) || !__has_include(<System/pthread_machdep.h>)". But it would be even simpler as an #else. Another option is to compute a macro named something like HAVE_PTHREAD_GETSPECIFIC_DIRECT and use that everywhere. > Source/bmalloc/bmalloc/VMAllocate.h:120 > + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); FREE is probably better than DONTNEED; DONTNEED pages will be paged out to disk under memory pressure, which is a waste for free malloc pages. > Source/bmalloc/bmalloc/VMAllocate.h:130 > + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); DONTNEED is wrong; to allocate physical pages, you want NORMAL.
Gyuyoung Kim
Comment 31 2015-01-29 00:22:28 PST
Comment on attachment 245447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245447&action=review >> Source/bmalloc/bmalloc/PerThread.h:82 >> +#if defined(__has_include) || defined(BUILDING_EFL__) > > This is wrong. To match the conditions above, it would need to read "#if !defined(__has_include) || !__has_include(<System/pthread_machdep.h>)". But it would be even simpler as an #else. > > Another option is to compute a macro named something like HAVE_PTHREAD_GETSPECIFIC_DIRECT and use that everywhere. Looks like below can support all ports. #if !defined(__has_include) || !__has_include(<System/pthread_machdep.h>) ... #else ... #endif >> Source/bmalloc/bmalloc/VMAllocate.h:120 >> + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); > > FREE is probably better than DONTNEED; DONTNEED pages will be paged out to disk under memory pressure, which is a waste for free malloc pages. Can MADV_FREE be used for linux as well as Mac OS ? It looks MADV_FREE can be used under OS_DARWIN. #if OS(DARWIN) #define HAVE_DISPATCH_H 1 #define HAVE_MADV_FREE 1 ... #endif void TCMalloc_SystemRelease(void* start, size_t length) { // MADV_FREE clears the modified bit on pages, which allows // them to be discarded immediately. #if HAVE(MADV_FREE) const int advice = MADV_FREE; #else const int advice = MADV_DONTNEED; #endif >> Source/bmalloc/bmalloc/VMAllocate.h:130 >> + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); > > DONTNEED is wrong; to allocate physical pages, you want NORMAL. Done.
Gyuyoung Kim
Comment 32 2015-01-29 00:29:24 PST
Comment on attachment 245447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245447&action=review >> Source/bmalloc/bmalloc/PerThread.h:57 >> +#if __has_include(<System/pthread_machdep.h>) > > Why did this have to change? EFL port doesn't recognize __has_include macro. So it causes build break. To avoid this build break, it seems that other files has used this style. AFAIK, __has_include is supported by Clang on LLVM. However EFL port doesn't support Clang compiler officially yet. For instance, at 35 line in Source/WebCore/crypto/CommonCryptoUtilities.h, #if defined(__has_include) #if __has_include(<CommonCrypto/CommonRSACryptor.h>) #include <CommonCrypto/CommonRSACryptor.h> #endif
Gyuyoung Kim
Comment 33 2015-01-29 00:31:21 PST
Created attachment 245613 [details] Patch
Gyuyoung Kim
Comment 34 2015-01-29 00:31:52 PST
Need to wait until EFL and GTK builds are fixed.
Csaba Osztrogonác
Comment 35 2015-01-29 00:33:55 PST
(In reply to comment #34) > Need to wait until EFL and GTK builds are fixed. feel free to pick it up: bug141027
Geoffrey Garen
Comment 36 2015-01-29 11:46:42 PST
Comment on attachment 245613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245613&action=review > Source/bmalloc/bmalloc/VMAllocate.h:120 > + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); Should be MADV_FREE.
Gyuyoung Kim
Comment 37 2015-01-29 17:13:05 PST
(In reply to comment #36) > Comment on attachment 245613 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245613&action=review > > > Source/bmalloc/bmalloc/VMAllocate.h:120 > > + SYSCALL(madvise(p, vmSize, MADV_DONTNEED)); > > Should be MADV_FREE. As I said comment #31, it looks MADV_FREE only can be used in Mac OS (DARWIN). IMHO, that's why TCMalloc has used MADV_DONTNEED instead of MADV_FREE in TCMalloc_SystemRelease(). If you know how to use MADV_FREE in other ports based on linux, please let me know. void TCMalloc_SystemRelease(void* start, size_t length) { // MADV_FREE clears the modified bit on pages, which allows // them to be discarded immediately. #if HAVE(MADV_FREE) const int advice = MADV_FREE; #else const int advice = MADV_DONTNEED; #endif
Carlos Alberto Lopez Perez
Comment 38 2015-01-29 18:10:13 PST
(In reply to comment #37) > If you know how to use MADV_FREE in other ports based on linux, please let me know. AFAIK, is not possible to use MADV_FREE on Linux. It seems that support for this feature is coming with Linux kernel version 3.20 [1]. But given that version 3.19 is still not released, and also taking into account the distribution release cycles, probably we would have to wait a few years until we can use this feature unconditionally on Linux. [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=831579f10b4000009972369acb54bae29ae3dedc
Gyuyoung Kim
Comment 39 2015-01-29 21:17:04 PST
Created attachment 245692 [details] Patch
Gyuyoung Kim
Comment 40 2015-01-30 06:27:32 PST
(In reply to comment #38) > (In reply to comment #37) > > If you know how to use MADV_FREE in other ports based on linux, please let me know. > > AFAIK, is not possible to use MADV_FREE on Linux. > > It seems that support for this feature is coming with Linux kernel version > 3.20 [1]. But given that version 3.19 is still not released, and also taking > into account the distribution release cycles, probably we would have to wait > a few years until we can use this feature unconditionally on Linux. > > > [1] > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/ > ?id=831579f10b4000009972369acb54bae29ae3dedc Geoffrey, what do you think about this ? Do you still think we should use MADV_FREE for EFL and GTK ports ?
Geoffrey Garen
Comment 41 2015-01-30 10:40:26 PST
Comment on attachment 245692 [details] Patch Sorry -- I missed your earlier comment. MADV_DONTNEED is OK. Looks like this patch failed to build on EFL.
Gyuyoung Kim
Comment 42 2015-01-30 17:15:19 PST
Created attachment 245763 [details] Patch
Gyuyoung Kim
Comment 43 2015-01-30 19:19:42 PST
Created attachment 245766 [details] Patch
Gyuyoung Kim
Comment 44 2015-01-30 20:31:36 PST
Comment on attachment 245766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245766&action=review > Source/cmake/WebKitHelpers.cmake:25 > + set(OLD_COMPILE_FLAGS "-Werror -Wno-error=attributes -Wno-error=unused-parameter ${OLD_COMPILE_FLAGS}") I would like to fix build breaks by "attributes" in following bugs.
Csaba Osztrogonác
Comment 45 2015-02-01 10:13:45 PST
(In reply to comment #44) > Comment on attachment 245766 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245766&action=review > > > Source/cmake/WebKitHelpers.cmake:25 > > + set(OLD_COMPILE_FLAGS "-Werror -Wno-error=attributes -Wno-error=unused-parameter ${OLD_COMPILE_FLAGS}") > > I would like to fix build breaks by "attributes" in following bugs. I don't like the idea to disable this warning everywhere because of 2 files. Let's see the warnings: ------------------------ ../../Source/bmalloc/bmalloc/Chunk.h:66:39: error: requested alignment 4096 is larger than 128 [-Werror=attributes] alignas(vmPageSize) char m_memory[]; ^ ../../Source/bmalloc/bmalloc/LargeChunk.h:70:39: error: requested alignment 4096 is larger than 128 [-Werror=attributes] alignas(vmPageSize) char m_memory[]; ^ I don't know if it is a false warning or it signals a real alignment bug. But we can mute it simple for this part only: +#if defined(__GNUC__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wattributes" alignas(vmPageSize) char m_memory[]; +#pragma GCC diagnostic pop +#endif If it is a false alarm, please justify why. But if it is a real bug, it should be fixed before landing the patch.
Gyuyoung Kim
Comment 46 2015-02-01 23:17:29 PST
Created attachment 245860 [details] Patch
Gyuyoung Kim
Comment 47 2015-02-01 23:20:33 PST
Comment on attachment 245860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245860&action=review > Source/bmalloc/bmalloc/Chunk.h:67 > alignas(vmPageSize) char m_memory[]; It looks there is issue when using alignas on gcc. However clang looks fine to use it. - http://stackoverflow.com/questions/15523537/alignas-specifier-vs-attribute-aligned-c11 So I change to use a char array which has vmPageSize for gcc. If this use has problem, please let me know.
Csaba Osztrogonác
Comment 48 2015-02-02 03:07:35 PST
Comment on attachment 245860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245860&action=review >> Source/bmalloc/bmalloc/Chunk.h:67 >> alignas(vmPageSize) char m_memory[]; > > It looks there is issue when using alignas on gcc. However clang looks fine to use it. > - http://stackoverflow.com/questions/15523537/alignas-specifier-vs-attribute-aligned-c11 > > So I change to use a char array which has vmPageSize for gcc. If this use has problem, please let me know. char x[4096] doesn't guarantee any alignment, I checked it on a small example with the stock GCC (4.8.3) on Ubuntu 14.04: class Foo { public: char a; int b; char c char d[4096]; }; Foo bar; bar.a: 0x7fff64e51520 bar.b: 0x7fff64e51524 bar.c: 0x7fff64e51528 bar.d: 0x7fff64e51529 --> not aligned at all But __attribute__ ((aligned(4096))) guarantees the proper alignment.
Gyuyoung Kim
Comment 49 2015-02-02 05:04:00 PST
Created attachment 245869 [details] Patch
Gyuyoung Kim
Comment 50 2015-02-02 05:05:12 PST
(In reply to comment #48) > Comment on attachment 245860 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245860&action=review > > >> Source/bmalloc/bmalloc/Chunk.h:67 > >> alignas(vmPageSize) char m_memory[]; > > > > It looks there is issue when using alignas on gcc. However clang looks fine to use it. > > - http://stackoverflow.com/questions/15523537/alignas-specifier-vs-attribute-aligned-c11 > > > > So I change to use a char array which has vmPageSize for gcc. If this use has problem, please let me know. > > char x[4096] doesn't guarantee any alignment, I checked it on > a small example with the stock GCC (4.8.3) on Ubuntu 14.04: > > class Foo > { > public: > char a; > int b; > char c > char d[4096]; > }; > > Foo bar; > > bar.a: 0x7fff64e51520 > bar.b: 0x7fff64e51524 > bar.c: 0x7fff64e51528 > bar.d: 0x7fff64e51529 --> not aligned at all > > But __attribute__ ((aligned(4096))) guarantees the proper alignment. Ossy, thank you for your nice comment. I missed it. I apply your suggestion to new patch. Please take a look !
Geoffrey Garen
Comment 51 2015-02-02 15:48:32 PST
> If it is a false alarm, please justify why. But if it is > a real bug, it should be fixed before landing the patch. This is a real bug.
Geoffrey Garen
Comment 52 2015-02-02 15:48:57 PST
Comment on attachment 245869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245869&action=review > Source/bmalloc/bmalloc/PerThread.h:35 > +#if defined(__has_include) > +#if __has_include(<System/pthread_machdep.h>) Can we put this on one line using &&?
Gyuyoung Kim
Comment 53 2015-02-02 21:35:48 PST
(In reply to comment #52) > Comment on attachment 245869 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245869&action=review > > > Source/bmalloc/bmalloc/PerThread.h:35 > > +#if defined(__has_include) > > +#if __has_include(<System/pthread_machdep.h>) > > Can we put this on one line using &&? Build break happens on EFL port when using it. Because EFL port doesn't support clang yet. When we support the clang, I will change to use "&&" for it. So I would like to keep current use at the moment.
Gyuyoung Kim
Comment 54 2015-02-02 21:39:56 PST
(In reply to comment #51) > > If it is a false alarm, please justify why. But if it is > > a real bug, it should be fixed before landing the patch. > > This is a real bug. As I said on comment #47, it looks there is issue when using *alignas* on gcc. However clang looks fine to use it. - http://stackoverflow.com/questions/15523537/alignas-specifier-vs-attribute-aligned-c11 Thus latest patch treats the attributes errors with warning only for *alignas* use. If you know how to solve this problem, please let me know.
Carlos Alberto Lopez Perez
Comment 55 2015-02-03 10:33:58 PST
(In reply to comment #53) > Build break happens on EFL port when using it. Because EFL port doesn't > support clang yet. When we support the clang, I will change to use "&&" for > it. So I would like to keep current use at the moment. IMHO: Supporting Clang is fine, but stopping to support GCC is not.
Csaba Osztrogonác
Comment 56 2015-02-03 10:36:46 PST
Comment on attachment 245869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245869&action=review > Source/bmalloc/bmalloc/Chunk.h:71 > +#if defined(__GNUC__) > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wattributes" > alignas(vmPageSize) char m_memory[]; > +#pragma GCC diagnostic pop > +#endif What was the problem with __attribute__ ((aligned(4096))) ?
Csaba Osztrogonác
Comment 57 2015-02-03 10:40:19 PST
(In reply to comment #55) > IMHO: Supporting Clang is fine, but stopping to support GCC is not. I agree, we shouldn't drop supporting GCC. At least not in the near future. (Otherwise EFL builds with clang fine, but not with Werror. Once I started fixing warnings to be able switch Werror on for clang too. Additionally one of the EFL jhbuild package has some issue with clang, but I can't remember which one. But building deps with gcc and building WebKit with clang works fine out-of-the-box. )
Gyuyoung Kim
Comment 58 2015-02-03 17:06:59 PST
Created attachment 245989 [details] Patch
Gyuyoung Kim
Comment 59 2015-02-03 19:56:23 PST
(In reply to comment #56) > Comment on attachment 245869 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245869&action=review > > > Source/bmalloc/bmalloc/Chunk.h:71 > > +#if defined(__GNUC__) > > +#pragma GCC diagnostic push > > +#pragma GCC diagnostic ignored "-Wattributes" > > alignas(vmPageSize) char m_memory[]; > > +#pragma GCC diagnostic pop > > +#endif > > What was the problem with __attribute__ ((aligned(4096))) ? Looks like no problem on that. Last patch uses it now.
Gyuyoung Kim
Comment 60 2015-02-04 18:34:20 PST
(In reply to comment #59) > (In reply to comment #56) > > Comment on attachment 245869 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=245869&action=review > > > > > Source/bmalloc/bmalloc/Chunk.h:71 > > > +#if defined(__GNUC__) > > > +#pragma GCC diagnostic push > > > +#pragma GCC diagnostic ignored "-Wattributes" > > > alignas(vmPageSize) char m_memory[]; > > > +#pragma GCC diagnostic pop > > > +#endif > > > > What was the problem with __attribute__ ((aligned(4096))) ? > > Looks like no problem on that. Last patch uses it now. Geoffrey and Ossy, any other comment on last patch ?
Csaba Osztrogonác
Comment 61 2015-02-06 02:57:41 PST
Comment on attachment 245989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245989&action=review I'm not an allocator expert, I only suggested some minor things and let the detailed review for others. Otherwise I don't think if we should enable bmalloc on linux until it can't provide memory statistics as tcmalloc does now - bug141247. > Source/bmalloc/bmalloc/Chunk.h:66 > + __attribute__((aligned(4096))) char m_memory[]; The problem here is that vmPageSize is 16384 on BPLATFORM(IOS) and 4096 everywhere else. Unfortunately GCC doesn't accept vmPageSize as attribute parameter: "error: requested alignment is not an integer constant" We have to duplicate these numbers and add compile time assert to guarantee that they are exactly same in Sizes.h and here. for example: #if BPLATFORM(IOS) char m_memory[] __attribute__((aligned(16384))); static_assert(vmPageSize == 16384, "vmPageSize and alignment must be same"); #else char m_memory[] __attribute__((aligned(4096))); static_assert(vmPageSize == 4096, "vmPageSize and alignment must be same"); #endif
Csaba Osztrogonác
Comment 62 2015-02-06 03:00:03 PST
Comment on attachment 245989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245989&action=review > Source/bmalloc/ChangeLog:19 > + -fpermitive build error on EFL and GTK port. nit: trailing whitespace
Carlos Garcia Campos
Comment 63 2015-02-06 03:16:15 PST
(In reply to comment #61) > Comment on attachment 245989 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245989&action=review > > I'm not an allocator expert, I only suggested some > minor things and let the detailed review for others. > > Otherwise I don't think if we should enable bmalloc on linux until > it can't provide memory statistics as tcmalloc does now - bug141247. If it's stable and gives much better performance, I don't think memory stats should be a blocker, at least not for GTK port.
Csaba Osztrogonác
Comment 64 2015-02-06 03:19:31 PST
(In reply to comment #63) > (In reply to comment #61) > > Comment on attachment 245989 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=245989&action=review > > > > I'm not an allocator expert, I only suggested some > > minor things and let the detailed review for others. > > > > Otherwise I don't think if we should enable bmalloc on linux until > > it can't provide memory statistics as tcmalloc does now - bug141247. > > If it's stable and gives much better performance, I don't think memory stats > should be a blocker, at least not for GTK port. Not a blocker? It would make impossible to catch memory regressions. I can understand if memory consumption isn't important, but can't agree. :)
Adrian Perez
Comment 65 2015-02-06 07:37:06 PST
Just a chiming in: I do also think that memory statistics are important to have. It shouldn't be hard to keep memory counters, and the overhead would be negligible. The gains from using bmalloc would still be noticeable even when keeping the memory counters around, and they can really be of help for finding regressions or spotting potential leaks without needing for full debug builds. Just my 2¢
Gyuyoung Kim
Comment 66 2015-02-06 07:44:09 PST
(In reply to comment #61) > Comment on attachment 245989 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245989&action=review > > I'm not an allocator expert, I only suggested some > minor things and let the detailed review for others. > > Otherwise I don't think if we should enable bmalloc on linux until > it can't provide memory statistics as tcmalloc does now - bug141247. I'm not sure when bmalloc can support memory stats though, I understand what is your concern. It would be nicer if we use bmalloc when it supports memory stats. My concern is nobody know when it can support memory stats. Do you think it will be done in near future ? If not, can't we land this patch with disabling bmalloc ? Then we can enable bmalloc as soon as it supports memory stats. > > Source/bmalloc/bmalloc/Chunk.h:66 > > + __attribute__((aligned(4096))) char m_memory[]; > > The problem here is that vmPageSize is 16384 on BPLATFORM(IOS) and 4096 > everywhere else. > > Unfortunately GCC doesn't accept vmPageSize as attribute parameter: > "error: requested alignment is not an integer constant" > We have to duplicate these numbers and add compile time assert > to guarantee that they are exactly same in Sizes.h and here. > > for example: > #if BPLATFORM(IOS) > char m_memory[] __attribute__((aligned(16384))); > static_assert(vmPageSize == 16384, "vmPageSize and alignment must be > same"); > #else > char m_memory[] __attribute__((aligned(4096))); > static_assert(vmPageSize == 4096, "vmPageSize and alignment must be > same"); > #endif I wonder how does Geoffrey think about this solution.
Gyuyoung Kim
Comment 67 2015-02-08 22:46:14 PST
Comment on attachment 245989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245989&action=review >>> Source/bmalloc/bmalloc/Chunk.h:66 >>> + __attribute__((aligned(4096))) char m_memory[]; >> >> The problem here is that vmPageSize is 16384 on BPLATFORM(IOS) and 4096 everywhere else. >> >> Unfortunately GCC doesn't accept vmPageSize as attribute parameter: >> "error: requested alignment is not an integer constant" >> We have to duplicate these numbers and add compile time assert >> to guarantee that they are exactly same in Sizes.h and here. >> >> for example: >> #if BPLATFORM(IOS) >> char m_memory[] __attribute__((aligned(16384))); >> static_assert(vmPageSize == 16384, "vmPageSize and alignment must be same"); >> #else >> char m_memory[] __attribute__((aligned(4096))); >> static_assert(vmPageSize == 4096, "vmPageSize and alignment must be same"); >> #endif > > I wonder how does Geoffrey think about this solution. > Unfortunately GCC doesn't accept vmPageSize as attribute parameter: > "error: requested alignment is not an integer constant" When I change to the line as below, there is no build error. __attribute__((aligned(vmPageSize))) char m_memory[]; I'm using GCC 4.9.1 version. What GCC version did this build error happen ?
Csaba Osztrogonác
Comment 68 2015-02-09 02:06:16 PST
(In reply to comment #67) > Comment on attachment 245989 [details] > Patch > > Unfortunately GCC doesn't accept vmPageSize as attribute parameter: > > "error: requested alignment is not an integer constant" > > When I change to the line as below, there is no build error. > __attribute__((aligned(vmPageSize))) char m_memory[]; > > I'm using GCC 4.9.1 version. What GCC version did this build error happen ? I got build error with GCC 4.8.2 which is the stock GCC in Ubuntu 14.04 LTS. Of course, it is possible to bump the minimumum required GCC version. I'm not against it, but the EFL and GTK port maintainers should decide if they want to drop GCC 4.8 support because of this minor issue or not. ( Additionally I checked Clang 3.5 - which is available for 14.04 - it is happy with __attribute__((aligned(vmPageSize))) too. )
Gyuyoung Kim
Comment 69 2015-02-09 04:12:57 PST
Created attachment 246265 [details] Patch
Gyuyoung Kim
Comment 70 2015-02-09 04:15:34 PST
(In reply to comment #68) > Of course, it is possible to bump the minimumum required GCC version. I'm > not against it, but the EFL and GTK port maintainers should decide if they > want to drop GCC 4.8 support because of this minor issue or not. I update your suggestion in last patch. Because I think it is too big issue to bump GCC version because of it. And I enable bmalloc on GTK port for now. Let's enable bmalloc for EFL port when it supports memory statistics.
Carlos Garcia Campos
Comment 71 2015-02-10 00:51:31 PST
I haven't said memory stats are not important, only that they shouldn't be a blocker. We could land the patch and make bmalloc the default one, but build with TCMalloc in the release bot. That way the release and perf bots would be using TCMalloc and we would get memory stats in perf bot, and the debug builds, development builds and production builds would use bmalloc (by default). What do you think?
Carlos Garcia Campos
Comment 72 2015-02-10 05:18:27 PST
hmm, Zan correctly pointed out that debug bot doesn't use tcmalloc, but system malloc.
Csaba Osztrogonác
Comment 73 2015-02-10 05:21:47 PST
(In reply to comment #71) > I haven't said memory stats are not important, only that they shouldn't be a > blocker. We could land the patch and make bmalloc the default one, but build > with TCMalloc in the release bot. That way the release and perf bots would > be using TCMalloc and we would get memory stats in perf bot, and the debug > builds, development builds and production builds would use bmalloc (by > default). What do you think? Good idea, maybe it is more than enough to use TCMalloc on the performance bots for now. And once bmalloc supports memory statistics, we can switch these bots too and remove TCMalloc.
Carlos Alberto Lopez Perez
Comment 74 2015-02-10 05:57:45 PST
(In reply to comment #73) > Good idea, maybe it is more than enough to use TCMalloc on the performance > bots for now. And once bmalloc supports memory statistics, we can switch > these bots too and remove TCMalloc. The GTK performance bot don't builds WebKit. It downloads the built product from the release bot. So, we would have to force TCMalloc on all the GTK release bots (build, tests and performance). That don't seems a good idea to me, since we would be testing one thing (TCMalloc), and shipping another different one (BMalloc). Maybe this can work better with the EFL performance bot, since this one (AFAIK) builds WebKit. So, if the EFL maintainers agree, we could force TCMalloc on the EFL performance bot until bug141247 is fixed. And on GTK (or MAC) we can rely on the memory stats from EFL in the meanwhile.
Gyuyoung Kim
Comment 75 2015-02-10 07:32:26 PST
(In reply to comment #74) > So, if the EFL maintainers agree, we could force TCMalloc on the EFL > performance bot until bug141247 is fixed. And on GTK (or MAC) we can rely on > the memory stats from EFL in the meanwhile. I already said similar suggestion in #comment 70. So I agree with this. Last patch enables bmalloc only for GTK port. When bug 141247 is fixed, I will enable bmalloc for EFL port as well. All agree ?
Carlos Garcia Campos
Comment 76 2015-02-10 10:18:09 PST
(In reply to comment #75) > (In reply to comment #74) > > > So, if the EFL maintainers agree, we could force TCMalloc on the EFL > > performance bot until bug141247 is fixed. And on GTK (or MAC) we can rely on > > the memory stats from EFL in the meanwhile. > > I already said similar suggestion in #comment 70. So I agree with this. Last > patch enables bmalloc only for GTK port. When bug 141247 is fixed, I will > enable bmalloc for EFL port as well. All agree ? I agree
Gyuyoung Kim
Comment 77 2015-02-10 20:10:16 PST
(In reply to comment #76) > (In reply to comment #75) > > (In reply to comment #74) > > > > > So, if the EFL maintainers agree, we could force TCMalloc on the EFL > > > performance bot until bug141247 is fixed. And on GTK (or MAC) we can rely on > > > the memory stats from EFL in the meanwhile. > > > > I already said similar suggestion in #comment 70. So I agree with this. Last > > patch enables bmalloc only for GTK port. When bug 141247 is fixed, I will > > enable bmalloc for EFL port as well. All agree ? > > I agree I file a new bug(Bug 141459) to enable bmalloc after fixing this and Bug 141247.
Carlos Garcia Campos
Comment 78 2015-02-11 00:14:12 PST
Comment on attachment 246265 [details] Patch Let's give it a try then, thanks you!
Adrian Perez
Comment 79 2015-02-11 00:54:08 PST
(In reply to comment #76) > (In reply to comment #75) > > (In reply to comment #74) > > > > > So, if the EFL maintainers agree, we could force TCMalloc on the EFL > > > performance bot until bug141247 is fixed. And on GTK (or MAC) we can rely on > > > the memory stats from EFL in the meanwhile. > > > > I already said similar suggestion in #comment 70. So I agree with this. Last > > patch enables bmalloc only for GTK port. When bug 141247 is fixed, I will > > enable bmalloc for EFL port as well. All agree ? > > I agree Yep, it sounds okay.
WebKit Commit Bot
Comment 80 2015-02-11 04:15:26 PST
Comment on attachment 246265 [details] Patch Clearing flags on attachment: 246265 Committed r179923: <http://trac.webkit.org/changeset/179923>
WebKit Commit Bot
Comment 81 2015-02-11 04:15:40 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 82 2015-02-11 05:47:32 PST
(In reply to comment #80) > Comment on attachment 246265 [details] > Patch > > Clearing flags on attachment: 246265 > > Committed r179923: <http://trac.webkit.org/changeset/179923> It broke the 32 bit x86 and ARM builds.
Csaba Osztrogonác
Comment 83 2015-02-11 05:50:42 PST
(In reply to comment #82) > (In reply to comment #80) > > Comment on attachment 246265 [details] > > Patch > > > > Clearing flags on attachment: 246265 > > > > Committed r179923: <http://trac.webkit.org/changeset/179923> > > It broke the 32 bit x86 and ARM builds. I'm working on EFL ARM fix.
Csaba Osztrogonác
Comment 84 2015-02-11 05:57:11 PST
new bug report to fix 32 bit builds: bug141472
Gyuyoung Kim
Comment 86 2015-02-11 18:35:47 PST
(In reply to comment #85) > Nice performance gains on the bots: > https://perf.webkit.org/ > #mode=charts&chartList=%5B%5B%22gtk%22%2C%22Parser%2Fhtml5-full- > render%3ATime%22%5D%5D > https://perf.webkit.org/ > #mode=charts&chartList=%5B%5B%22gtk%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATo > tal%22%5D%5D WOW, nice result ! I hope to enable bmalloc for EFL port as soon as possible. To do that, we have to fix bug 141247 ASAP. Let me help to fix it as well.
Geoffrey Garen
Comment 87 2015-02-12 16:34:05 PST
> > #if BPLATFORM(IOS) > > char m_memory[] __attribute__((aligned(16384))); > > static_assert(vmPageSize == 16384, "vmPageSize and alignment must be > > same"); > > #else > > char m_memory[] __attribute__((aligned(4096))); > > static_assert(vmPageSize == 4096, "vmPageSize and alignment must be > > same"); > > #endif > > I wonder how does Geoffrey think about this solution. This is pretty ugly code. It's a shame that we have to do this in order to support an obsolete compiler. :( Is there any reasonable version of GCC that supports the C++ alignas feature? What is the cost of requiring a modern GCC on Linux?
Gyuyoung Kim
Comment 88 2015-02-12 17:04:05 PST
(In reply to comment #87) > > > #if BPLATFORM(IOS) > > > char m_memory[] __attribute__((aligned(16384))); > > > static_assert(vmPageSize == 16384, "vmPageSize and alignment must be > > > same"); > > > #else > > > char m_memory[] __attribute__((aligned(4096))); > > > static_assert(vmPageSize == 4096, "vmPageSize and alignment must be > > > same"); > > > #endif > > > > I wonder how does Geoffrey think about this solution. > > This is pretty ugly code. It's a shame that we have to do this in order to > support an obsolete compiler. :( > > Is there any reasonable version of GCC that supports the C++ alignas > feature? What is the cost of requiring a modern GCC on Linux? In comment #67, we already talked about this. Below C++ aligns can be supported by GCC 4.9.1. However we thought that to bump GCC version is too big work because of it. __attribute__((aligned(vmPageSize))) char m_memory[]; However if GTK can support over gcc 4.9.1, I also would try to bump gcc version for EFL buildbot and ews.
Carlos Alberto Lopez Perez
Comment 89 2015-02-12 17:22:27 PST
GCC 4.8 is what Ubuntu 14.04 LTS ships. Raising the required GCC version because of this will cause troubles to every developer using the last Ubuntu LTS (I don't have numbers, but I think there are more than a few of them). And given that by using this 4-line ifdef section we can make the code work with GCC 4.8, I don't think is reasonable to raise the GCC version just to avoid the small ifdef.
Carlos Garcia Campos
Comment 90 2015-02-12 23:26:25 PST
(In reply to comment #89) > GCC 4.8 is what Ubuntu 14.04 LTS ships. > > Raising the required GCC version because of this will cause troubles to > every developer using the last Ubuntu LTS (I don't have numbers, but I think > there are more than a few of them). > > And given that by using this 4-line ifdef section we can make the code work > with GCC 4.8, I don't think is reasonable to raise the GCC version just to > avoid the small ifdef. I agree, but I'm afraid this is the typical code that will be there forever, unless we open a bug and add a FIXME in the code pointing to the bug, to remove the workaround once all linux ports bump the GCC requirements.
Gyuyoung Kim
Comment 91 2015-02-12 23:40:30 PST
(In reply to comment #90) > (In reply to comment #89) > > GCC 4.8 is what Ubuntu 14.04 LTS ships. > > > > Raising the required GCC version because of this will cause troubles to > > every developer using the last Ubuntu LTS (I don't have numbers, but I think > > there are more than a few of them). > > > > And given that by using this 4-line ifdef section we can make the code work > > with GCC 4.8, I don't think is reasonable to raise the GCC version just to > > avoid the small ifdef. > > I agree, but I'm afraid this is the typical code that will be there forever, > unless we open a bug and add a FIXME in the code pointing to the bug, to > remove the workaround once all linux ports bump the GCC requirements. I agree with it as well. At least "FIXME" should be there. I upload a fix to Bug 141459 for it.
Gyuyoung Kim
Comment 92 2015-02-12 23:41:54 PST
(In reply to comment #91) > I agree with it as well. At least "FIXME" should be there. I upload a fix to > Bug 141459 for it. Oops, sorry Bug 141556 is correct.
Csaba Osztrogonác
Comment 93 2015-02-13 00:38:28 PST
(In reply to comment #87) > This is pretty ugly code. It's a shame that we have to do this in order to > support an obsolete compiler. :( > > Is there any reasonable version of GCC that supports the C++ alignas > feature? What is the cost of requiring a modern GCC on Linux? Considering GCC 4.8.2 to be an obsolete compiler is ridiculous, it was released at October 16, 2013 - https://gcc.gnu.org/gcc-4.8/ and the latest LTS Ubuntu was released at 2014-04-17. (https://wiki.ubuntu.com/TrustyTahr/ReleaseNotes) Do you also plan to make Apple to bump to MSVC2015 once it is released? Just to remove this ugly workaround - http://trac.webkit.org/changeset/179993 ? I could point out many Mac/Windows/Linux workaround just to make compiler happy, but I don't think if removing all of these workarounds would make WebKit better, faster, more stable, etc. But I do support bumping compiler version if we can profit from the bumping, for example: get better performance or stability, we can use more modern language structure, etc. But not for a 4 lines workaround. If the majority agree that bumping to GCC 4.9 is reasonable, I'm not against it, but need a little bit time to get/build and _test_ newer GCC for our ARM bots. We didn't bump to 4.9.1 because of a compiler bug (unable to build one of the source file in WebKit), maybe it is already fixed in the latest 4.9.2 release, let me check it.
Csaba Osztrogonác
Comment 94 2015-02-13 07:05:41 PST
Just a note, the build fails with GCC 4.9.1 (at least on ARM, haven't checked on x86) with the following compiler error: ../../Source/WebCore/svg/SVGPathElement.h: In member function 'virtual WebCore::FloatRect WebCore::SVGPathElement::_ZTv0_n24_N7WebCore14SVGPathElement7getBBoxENS_12SVGLocatable19StyleUpdateStrategyE(WebCore::SVGLocatable::StyleUpdateStrategy)': ../../Source/WebCore/svg/SVGPathElement.h:94:23: internal compiler error: in expand_expr_addr_expr_1, at expr.c:7669
Michael Catanzaro
Comment 95 2015-02-13 07:40:25 PST
We build the with the latest GCC on ARM for Fedora (in fact I'm trying right now to find out if an ICE in GCC 5 has been fixed), and have no problems with GCC 4.9.2. It seems to fix several ICEs over 4.9.1: https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=RESOLVED&resolution=FIXED&target_milestone=4.9.2
Csaba Osztrogonác
Comment 96 2015-02-13 10:12:04 PST
(In reply to comment #94) > Just a note, the build fails with GCC 4.9.1 (at least on ARM, > haven't checked on x86) with the following compiler error: > > ../../Source/WebCore/svg/SVGPathElement.h: In member function 'virtual > WebCore::FloatRect > WebCore::SVGPathElement:: > _ZTv0_n24_N7WebCore14SVGPathElement7getBBoxENS_12SVGLocatable19StyleUpdateStr > ategyE(WebCore::SVGLocatable::StyleUpdateStrategy)': > ../../Source/WebCore/svg/SVGPathElement.h:94:23: internal compiler error: in > expand_expr_addr_expr_1, at expr.c:7669 I tested GCC 4.9.2 and Linaro 4.9.3 20150113 (prerelease) too and the bug is still valid. ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61207 ) Of course, it is possible to try to workaround in the cmake project to build this cpp with O2 instead of O3, but it isn't reasonable just to remove a not so good looking compiler workaround which doesn't change the behaviour at all.
Michael Catanzaro
Comment 97 2015-02-13 10:19:13 PST
OK, we use -O2, that's probably why we did not hit it.
Geoffrey Garen
Comment 98 2015-02-13 10:40:18 PST
> Raising the required GCC version because of this will cause troubles to > every developer using the last Ubuntu LTS (I don't have numbers, but I think > there are more than a few of them). Yes, I saw the claim that using a newer GCC would "cause troubles". What I'm asking is, what are the troubles, and are they solvable? Is the trouble just the Linux WebKit developers will need to install a new version of GCC? Or is there something deeper?
Geoffrey Garen
Comment 99 2015-02-13 10:43:39 PST
> Do you also plan to make Apple to bump to MSVC2015 once it is released? Yes. > I could point out many Mac/Windows/Linux workaround just to make compiler > happy, but I don't think if removing all of these workarounds would make > WebKit better, faster, more stable, etc. I guess we disagree on this point: I prefer to work with readable and writable code.
Csaba Osztrogonác
Comment 100 2015-02-13 10:46:38 PST
(In reply to comment #90) > (In reply to comment #89) > > GCC 4.8 is what Ubuntu 14.04 LTS ships. > > > > Raising the required GCC version because of this will cause troubles to > > every developer using the last Ubuntu LTS (I don't have numbers, but I think > > there are more than a few of them). > > > > And given that by using this 4-line ifdef section we can make the code work > > with GCC 4.8, I don't think is reasonable to raise the GCC version just to > > avoid the small ifdef. > > I agree, but I'm afraid this is the typical code that will be there forever, > unless we open a bug and add a FIXME in the code pointing to the bug, to > remove the workaround once all linux ports bump the GCC requirements. I don't think if it is common to ask bigger alignment than 128 bytes, the allocator is one of the few exceptions when we need so big alignment.
Csaba Osztrogonác
Comment 101 2015-02-13 11:00:18 PST
(In reply to comment #99) > > I could point out many Mac/Windows/Linux workaround just to make compiler > > happy, but I don't think if removing all of these workarounds would make > > WebKit better, faster, more stable, etc. > > I guess we disagree on this point: I prefer to work with readable and > writable code. No, we agree more or less, I prefer nice codes too, but not at whatever cost. I also landed many cleanups, refactoring, removing obsolete codes, ifdefs, unnecessary compiler workarounds when we could do it without pain. I simply don't understand why we should pay a very high cost to remove a 4 lines simple workaround in a code which won't be read/touched except some developers. But bumping the compiler version would affect much more developers and would need another compiler workaround which would cause worse code at least in one file. (I hope we can agree that upgrading a compiler isn't only 2 clicks and O3 produces better code than O2.)
Michael Catanzaro
Comment 102 2015-02-13 11:29:49 PST
(In reply to comment #98) > Is the trouble just the Linux WebKit developers will need to install a new > version of GCC? Or is there something deeper? Well not really, but I think we have a cultural difference here: Linux developers expect the compiler (and pretty much everything except what they are developing) to be provided by the operating system, so bumping the compiler version roughly equates to dropping support for compiling on that operating system. (You can build your compiler yourself of course, but that's somewhat primitive; I would just upgrade to a newer OS as need be.) So what we're really talking about here is whether to drop support for Ubuntu 14.04, which is very popular and so not an insignificant consideration. The consensus in this topic seems to be that we're willing to do this for a significant compiler improvement, but maybe not something minor like this.
Note You need to log in before you can comment on or make changes to this bug.