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.
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
Created attachment 244340 [details] Very very WIP
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.
(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.
Created attachment 244516 [details] WIP-2
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
Created attachment 244840 [details] WIP-3 with Zan's change
(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.
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.
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.
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.
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?
Created attachment 245276 [details] WIP-4
(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 ?
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).
Created attachment 245294 [details] WIP-5
(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.
Created attachment 245305 [details] WIP-6
Created attachment 245306 [details] Patch
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.
Created attachment 245321 [details] Patch
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.
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.
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;
Created attachment 245352 [details] Patch for ews
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)".
Created attachment 245395 [details] Patch for ews
Created attachment 245447 [details] Patch
Though this patch may have still problems, it seems to me that this patch is ready to get official review.
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.
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.
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
Created attachment 245613 [details] Patch
Need to wait until EFL and GTK builds are fixed.
(In reply to comment #34) > Need to wait until EFL and GTK builds are fixed. feel free to pick it up: bug141027
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.
(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
(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
Created attachment 245692 [details] Patch
(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 ?
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.
Created attachment 245763 [details] Patch
Created attachment 245766 [details] Patch
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.
(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.
Created attachment 245860 [details] Patch
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.
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.
Created attachment 245869 [details] Patch
(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 !
> 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.
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 &&?
(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.
(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.
(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.
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))) ?
(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. )
Created attachment 245989 [details] Patch
(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.
(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 ?
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
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
(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.
(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. :)
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¢
(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.
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 ?
(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. )
Created attachment 246265 [details] Patch
(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.
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?
hmm, Zan correctly pointed out that debug bot doesn't use tcmalloc, but system malloc.
(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.
(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.
(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 ?
(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
(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.
Comment on attachment 246265 [details] Patch Let's give it a try then, thanks you!
(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.
Comment on attachment 246265 [details] Patch Clearing flags on attachment: 246265 Committed r179923: <http://trac.webkit.org/changeset/179923>
All reviewed patches have been landed. Closing bug.
(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.
(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.
new bug report to fix 32 bit builds: bug141472
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%3ATotal%22%5D%5D
(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.
> > #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 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.
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.
(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.
(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.
(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.
(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.
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
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
(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.
OK, we use -O2, that's probably why we did not hit it.
> 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?
> 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.
(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.
(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.)
(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.