Bug 27029 – [Qt] Allow custom memory allocation control in the whole JSCore

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Zoltan Horvath]

I got a request from Eric to prepare a whole patch for JSCore instead of sending single class inheritance patches.

I have some problems and we need to clear up FastAllocBase inheritance policies.

It's easy to inherit from a single classes which instantiated by 'new' and don't have super classes. It's clear.

At multiple inheritance case the policy was to go up as far as I can in the inheritance tree and inherite the superclass from FastAllocBase (bug #20422). 
The problem was in the case of Noncopyable, if I inherited Noncopyable from FastAllocBase it occured multiple new definitions and build errors (ambigous 'new' using). 
Further problems come up with e.g. JSCell, because it's also inherited from Noncopyable and overrides operator new because of using GC, so it's also cause conflicts.
What should be the policy? Remove that Noncopyable inheritances which causes conflicts or push FastAllocBase as far as I can in the inheritance tree? (In the second case there'll be many more inherites from FastAllocBase)

Thanks,

Zoltan

[Darin Adler]

(In reply to comment #0)
> The problem was in the case of Noncopyable, if I inherited Noncopyable from
> FastAllocBase it occured multiple new definitions and build errors (ambigous
> 'new' using). 
> Further problems come up with e.g. JSCell, because it's also inherited from
> Noncopyable and overrides operator new because of using GC, so it's also cause
> conflicts.

I think we can create two different Noncopyable. One for most uses that does include FastAllocBase, and the other for the few cases that customize operator new.

And if there are few enough cases of the non-FastAllocBase Noncopyable, then we could just do those by hand, rather than using a base class. But I'd prefer to use a base class if it's more than, say, 3 or 4 classes overall.

Maybe NoncopyableCustomAllocated as the name for the new Noncopyable variant? And put it in Noncopyable.h?

[Zoltan Horvath]

Created attachment 39982 [details]
proposed patch

This is the patch to enable TCmalloc for the Qt port. I tested it on Linux and Windows also and it works well.

Qt-linux performance results:

SunSpider
System malloc 769.48 +/- 1 %
TCmalloc 762.66 +/- 0.7 %

TC 0.8% faster
-------------------------------

V8
System malloc 3580.2 +/- 1.7 %
TCmalloc 3507.6 +/- 2.2 %

TC 2.1% faster
-------------------------------

WindScorpion

System malloc 18722.3 +/- 1.5 %
TCmalloc 17435.1 +/- 0.7%

TC 6.9% faster

[Eric Seidel (no email)]

Comment on attachment 39982 [details]
proposed patch

Why does this not just blow out the stack?

+static void sleep(unsigned seconds)
+{
+    sleep(seconds * 1000);
+}

Is one of them in WTF?  If so, then the other should use :: to make it explicit that it's a root namespace call.

Also the sleep call seems unrelated to the rest of the patch, or at least is not explained in the ChangeLog.

diff likes newlines at end of files:
+DEFINES+=USE_SYSTEM_MALLOC
\ No newline at end of file
(so does GCC)

[Zoltan Horvath]

Created attachment 40066 [details]
updated proposed patch


> Why does this not just blow out the stack?

It follows the implementation of the windows version:

#if PLATFORM(WIN)
static void sleep(unsigned seconds)
{
    ::Sleep(seconds * 1000);
}
#endif

> Is one of them in WTF?  If so, then the other should use :: to make it explicit
> that it's a root namespace call.

No it isn't. I put :: in.

> Also the sleep call seems unrelated to the rest of the patch, or at least is
> not explained in the ChangeLog.

> diff likes newlines at end of files:
> +DEFINES+=USE_SYSTEM_MALLOC
> \ No newline at end of file
> (so does GCC)

Done.

[Zoltan Horvath]

Created attachment 40067 [details]
updated proposed patch

[Simon Hausmann]

Cool stuff, I'm going to push your patch into our build farm to see if it affects any of the more exotic platforms

[Simon Hausmann]

Comment on attachment 40067 [details]
updated proposed patch

After trying it out this patch does not compile with mingw g++:

..\JavaScriptCore\wtf\FastMalloc.cpp:2289: error: '::sleep' has not been declared

[Zoltan Horvath]

Created attachment 40173 [details]
updated proposed patch (tested on win also)

I added now the win related parts of the patch and I tested it with MinGW-3.4.5 and with MinGW-4.4.0, both works well.

[Simon Hausmann]

(In reply to comment #8)
> Created an attachment (id=40173) [details]
> updated proposed patch (tested on win also)
> 
> I added now the win related parts of the patch and I tested it with MinGW-3.4.5
> and with MinGW-4.4.0, both works well.

Thanks! I admit I'm concerned that the next platform it'll break might be Symbian (which we could fix from here). But I just had another idea: Would it make sense to use QThread::sleep() here? That would certainly address the portability concerns.

I admit I don't know if this work as intended though, given that sleep(3) appears to suspect the process while QThread::sleep() only suspends the current thread.

What do you think?

[Simon Hausmann]

I see that TCSystemMalloc.cpp has compilation problems.

On Symbian:

wtf\tcsystemalloc.cpp", line 126: Error: #20: identifier "sbrk" is undefined

On Windows CE:

wtf\TCSystemAlloc.cpp(37) : fatal error C1083: Cannot open include file: 'fcntl.h': No such file or directory

Perhaps fastmalloc should be whitelisted first?

[Simon Hausmann]

I think for Symbian this will require a custom alloc implementation, as neither mmap nor VirtualFree are available. I somehow feel that sbrk isn't available either :)

Another argument for whitelisting though, as it certainly appears to be usable on linux/windows/mac.

[Simon Hausmann]

Comment on attachment 40173 [details]
updated proposed patch (tested on win also)

I'll say r- as it will break the windows ce and symbian builds. But I think that should be easy to fix with a white or blacklist.

Thanks for the quick win32 fix though :)

[Zoltan Horvath]

Created attachment 40288 [details]
updated proposed patch

Unfortunately, I don't have WinCE and Symbian, yet. :)
I put a whitelist/blacklist(?) into WebKit.pri and add extra checks to FastMalloc.cpp also.

[Eric Seidel (no email)]

Comment on attachment 40288 [details]
updated proposed patch

I think you want PLATFORM(DARWIN) not PLATFORM(MAC).  Yes, I know.  It's stupidly confusing.  Maciej supposedly has plans to fix this all some day.

[Zoltan Horvath]

Created attachment 40348 [details]
proposed patch for custom allocation control in JSCore

> I think you want PLATFORM(DARWIN) not PLATFORM(MAC).  Yes, I know.  It's
> stupidly confusing.  Maciej supposedly has plans to fix this all some day.

Yes, I was confused about MAC or DARWIN, but you are right! Thanks. As Platform.h contains we have to use DARWIN. I changed.

Btw, what will be Maciej's fix? Change DARWIN to MAC? :)

Patch has been updated!

[Zoltan Horvath]

Created attachment 40364 [details]
updated proposed patch

It has been updated based on Simon's comments.

[Simon Hausmann]

Comment on attachment 40364 [details]
updated proposed patch


> +#if PLATFORM(QT) && PLATFORM(UNIX)
> +#include <unistd.h>
> +#endif
>  #if COMPILER(MSVC)
>  #ifndef WIN32_LEAN_AND_MEAN
>  #define WIN32_LEAN_AND_MEAN
> @@ -2274,12 +2277,18 @@ static inline TCMalloc_PageHeap* getPage
>  #define pageheap getPageHeap()
>  
>  #if USE_BACKGROUND_THREAD_TO_SCAVENGE_MEMORY
> -#if PLATFORM(WIN)
> +#if PLATFORM(WIN_OS)
>  static void sleep(unsigned seconds)
>  {
>      ::Sleep(seconds * 1000);
>  }
>  #endif
> +#if PLATFORM(QT) && PLATFORM(UNIX)
> +static void sleep(unsigned seconds)
> +{
> +    ::sleep(seconds);
> +}
> +#endif

On a second thought, why is the sleep() overload needed here only for the Qt port?

The Gtk+ port builds TCSystemMalloc.cpp and doesn't seem to need the sleep() forward
function.

[Zoltan Horvath]

Created attachment 40448 [details]
updated proposed patch

Patch has been updated based on Simon's comments from IRC.

[Zoltan Horvath]

Landed in 48976.

http://trac.webkit.org/changeset/48976/