Bug 27029 - [Qt] Allow custom memory allocation control in the whole JSCore
Summary: [Qt] Allow custom memory allocation control in the whole JSCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-07-07 08:10 PDT by Zoltan Horvath
Modified: 2009-10-01 22:27 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (4.68 KB, patch)
2009-09-23 02:47 PDT, Zoltan Horvath
eric: review-
Details | Formatted Diff | Diff
updated proposed patch (6.34 KB, patch)
2009-09-24 08:19 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (5.00 KB, patch)
2009-09-24 08:32 PDT, Zoltan Horvath
hausmann: review-
Details | Formatted Diff | Diff
updated proposed patch (tested on win also) (5.25 KB, patch)
2009-09-26 07:45 PDT, Zoltan Horvath
hausmann: review-
Details | Formatted Diff | Diff
updated proposed patch (5.39 KB, patch)
2009-09-29 00:49 PDT, Zoltan Horvath
eric: review-
Details | Formatted Diff | Diff
proposed patch for custom allocation control in JSCore (5.42 KB, patch)
2009-09-30 00:35 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (5.38 KB, patch)
2009-09-30 05:47 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (5.13 KB, patch)
2009-10-01 09:15 PDT, Zoltan Horvath
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2009-07-07 08:10:10 PDT
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 Darin Adler 2009-07-09 09:29:04 PDT
(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 Zoltan Horvath 2009-09-23 02:47:46 PDT
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
Comment 3 Eric Seidel (no email) 2009-09-23 17:14:41 PDT
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)
Comment 4 Zoltan Horvath 2009-09-24 08:19:49 PDT
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.
Comment 5 Zoltan Horvath 2009-09-24 08:32:16 PDT
Created attachment 40067 [details]
updated proposed patch
Comment 6 Simon Hausmann 2009-09-25 04:11:47 PDT
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 Simon Hausmann 2009-09-25 05:35:24 PDT
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
Comment 8 Zoltan Horvath 2009-09-26 07:45:02 PDT
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.
Comment 9 Simon Hausmann 2009-09-27 11:45:49 PDT
(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?
Comment 10 Simon Hausmann 2009-09-27 13:23:05 PDT
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 Simon Hausmann 2009-09-27 13:30:30 PDT
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 Simon Hausmann 2009-09-27 13:31:42 PDT
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 :)
Comment 13 Zoltan Horvath 2009-09-29 00:49:19 PDT
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.
Comment 14 Eric Seidel (no email) 2009-09-29 14:08:39 PDT
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.
Comment 15 Zoltan Horvath 2009-09-30 00:35:08 PDT
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!
Comment 16 Zoltan Horvath 2009-09-30 05:47:34 PDT
Created attachment 40364 [details]
updated proposed patch

It has been updated based on Simon's comments.
Comment 17 Simon Hausmann 2009-10-01 07:28:25 PDT
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.
Comment 18 Zoltan Horvath 2009-10-01 09:15:53 PDT
Created attachment 40448 [details]
updated proposed patch

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

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