Bug 27029

Summary: [Qt] Allow custom memory allocation control in the whole JSCore
Product: WebKit Reporter: Zoltan Horvath <zoltan@webkit.org>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: darin@apple.com, hausmann@webkit.org, kenneth@webkit.org, ricardo.salveti@openbossa.org
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
proposed patch
eric: review-
updated proposed patch
none
updated proposed patch
hausmann: review-
updated proposed patch (tested on win also)
hausmann: review-
updated proposed patch
eric: review-
proposed patch for custom allocation control in JSCore
none
updated proposed patch
none
updated proposed patch hausmann: review+

Description From 2009-07-07 08:10:10 PST
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
------- Comment #1 From 2009-07-09 09:29:04 PST -------
(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?
------- Comment #2 From 2009-09-23 02:47:46 PST -------
Created an attachment (id=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
------- Comment #3 From 2009-09-23 17:14:41 PST -------
(From update of attachment 39982 [details])
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)
------- Comment #4 From 2009-09-24 08:19:49 PST -------
Created an attachment (id=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.
------- Comment #5 From 2009-09-24 08:32:16 PST -------
Created an attachment (id=40067) [details]
updated proposed patch
------- Comment #6 From 2009-09-25 04:11:47 PST -------
Cool stuff, I'm going to push your patch into our build farm to see if it affects any of the more exotic platforms
------- Comment #7 From 2009-09-25 05:35:24 PST -------
(From update of attachment 40067 [details])
After trying it out this patch does not compile with mingw g++:

..\JavaScriptCore\wtf\FastMalloc.cpp:2289: error: '::sleep' has not been declared
------- Comment #8 From 2009-09-26 07:45:02 PST -------
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.
------- Comment #9 From 2009-09-27 11:45:49 PST -------
(In reply to comment #8)
> Created an attachment (id=40173) [details] [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?
------- Comment #10 From 2009-09-27 13:23:05 PST -------
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?
------- Comment #11 From 2009-09-27 13:30:30 PST -------
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.
------- Comment #12 From 2009-09-27 13:31:42 PST -------
(From update of attachment 40173 [details])
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 :)
------- Comment #13 From 2009-09-29 00:49:19 PST -------
Created an attachment (id=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.
------- Comment #14 From 2009-09-29 14:08:39 PST -------
(From update of attachment 40288 [details])
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.
------- Comment #15 From 2009-09-30 00:35:08 PST -------
Created an attachment (id=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!
------- Comment #16 From 2009-09-30 05:47:34 PST -------
Created an attachment (id=40364) [details]
updated proposed patch

It has been updated based on Simon's comments.
------- Comment #17 From 2009-10-01 07:28:25 PST -------
(From update of attachment 40364 [details])

> +#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.
------- Comment #18 From 2009-10-01 09:15:53 PST -------
Created an attachment (id=40448) [details]
updated proposed patch

Patch has been updated based on Simon's comments from IRC.
------- Comment #19 From 2009-10-01 22:27:55 PST -------
Landed in 48976.

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