Bug 77013

Summary: [BlackBerry] Implement OSAllocator::commit/decommit
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, milian.wolff, staikos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
the patch
none
new solution none

Description Yong Li 2012-01-25 08:39:41 PST
BlackBerry port should also support decommitting virtual memory.
Comment 1 Yong Li 2012-01-25 08:42:59 PST
Created attachment 123947 [details]
the patch
Comment 2 Rob Buis 2012-01-25 08:52:40 PST
Comment on attachment 123947 [details]
the patch

LGTM.
Comment 3 WebKit Review Bot 2012-01-25 10:01:30 PST
Comment on attachment 123947 [details]
the patch

Attachment 123947 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11242851

New failing tests:
media/audio-garbage-collect.html
Comment 4 Yong Li 2012-01-25 10:49:24 PST
(In reply to comment #3)
> (From update of attachment 123947 [details])
> Attachment 123947 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11242851
> 
> New failing tests:
> media/audio-garbage-collect.html

cannot be due to this patch.
Comment 5 WebKit Review Bot 2012-01-25 12:52:57 PST
Comment on attachment 123947 [details]
the patch

Clearing flags on attachment: 123947

Committed r105918: <http://trac.webkit.org/changeset/105918>
Comment 6 WebKit Review Bot 2012-01-25 12:53:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Yong Li 2012-03-19 11:57:19 PDT
Actually we should implement this in another way
Comment 8 Yong Li 2012-03-19 12:12:38 PDT
Created attachment 132622 [details]
new solution
Comment 9 Rob Buis 2012-03-19 13:16:10 PDT
Comment on attachment 132622 [details]
new solution

LGTM.
Comment 10 WebKit Review Bot 2012-03-19 13:39:31 PDT
Comment on attachment 132622 [details]
new solution

Clearing flags on attachment: 132622

Committed r111234: <http://trac.webkit.org/changeset/111234>
Comment 11 WebKit Review Bot 2012-03-19 13:39:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Milian Wolff 2012-06-13 08:18:58 PDT
Hey there,

I am wondering about the last patch: What QNX are you working on? On QNX6 I cannot find an implementation of MADV_FREE_REUSABLE nor MADV_FREE_REUSE, which you apparently do have, considering that your patch contains:

#define HAVE_MADV_FREE_REUSE 1
#define HAVE_MADV_FREE 1 

With this in, I cannot compile QtWebKit for QNX6, should this maybe be disabled to read #define HAVE_MADV_FREE(_REUSE) 0?
Comment 13 Yong Li 2012-06-13 08:36:32 PDT
(In reply to comment #12)
> Hey there,
> 
> I am wondering about the last patch: What QNX are you working on? On QNX6 I cannot find an implementation of MADV_FREE_REUSABLE nor MADV_FREE_REUSE, which you apparently do have, considering that your patch contains:
> 
> #define HAVE_MADV_FREE_REUSE 1
> #define HAVE_MADV_FREE 1 
> 
> With this in, I cannot compile QtWebKit for QNX6, should this maybe be disabled to read #define HAVE_MADV_FREE(_REUSE) 0?

Try this one: http://trac.webkit.org/changeset/111234

QNX doesn't have madv_free/free_reuse. But I want to turn them on so we know where we should add our alternative implementation.
Comment 14 Milian Wolff 2012-06-13 08:53:37 PDT
Hey there,

the patchset you link to above does not cover the use case in TCSystemAlloc.cpp:

/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp: In function 'void TCMalloc_SystemRelease(void*, std::size_t)':
/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:390: error: 'MADV_FREE_REUSABLE' was not declared in this scope
/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:390: error: 'madvise' was not declared in this scope
/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp: In function 'void TCMalloc_SystemCommit(void*, std::size_t)':
/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:486: error: 'MADV_FREE_REUSE' was not declared in this scope
/home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:486: error: 'madvise' was not declared in this scope

Did you miss that, or is something going wrong on my side?
Comment 15 Yong Li 2012-06-13 09:05:35 PDT
(In reply to comment #14)
> Hey there,
> 
> the patchset you link to above does not cover the use case in TCSystemAlloc.cpp:
> 
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp: In function 'void TCMalloc_SystemRelease(void*, std::size_t)':
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:390: error: 'MADV_FREE_REUSABLE' was not declared in this scope
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:390: error: 'madvise' was not declared in this scope
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp: In function 'void TCMalloc_SystemCommit(void*, std::size_t)':
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:486: error: 'MADV_FREE_REUSE' was not declared in this scope
> /home/milian/projects/qt5/qtwebkit/Source/WTF/wtf/TCSystemAlloc.cpp:486: error: 'madvise' was not declared in this scope
> 
> Did you miss that, or is something going wrong on my side?

We don't use TCSystemAlloc.

So 2 options I can see here:

1. turn off HAVE_MADV_FREE and HAVE_MADV_FREE_REUSE. However the memory allocator won't be able to decommit free pages (return RAM to system).
2. Add an implementation as I did in r111234 for TCSystemAlloc.
Comment 16 Milian Wolff 2012-06-13 09:14:29 PDT
Ah ok,

so maybe we should change this block:

#if PLATFORM(BLACKBERRY)
#define USE_SYSTEM_MALLOC 1
#define WTF_USE_MERSENNE_TWISTER_19937 1
#define WTF_USE_SKIA 1
#endif

to use 

#if OS(QNX) || PLATFORM(BLACKBERRY)
...
#endif

? What do you think?
Comment 17 Yong Li 2012-06-13 09:25:49 PDT
(In reply to comment #16)
> Ah ok,
> 
> so maybe we should change this block:
> 
> #if PLATFORM(BLACKBERRY)
> #define USE_SYSTEM_MALLOC 1
> #define WTF_USE_MERSENNE_TWISTER_19937 1
> #define WTF_USE_SKIA 1
> #endif
> 
> to use 
> 
> #if OS(QNX) || PLATFORM(BLACKBERRY)
> ...
> #endif
> 
> ? What do you think?

Not sure for the other 2. But it is OK to always use USE_SYSTEM_MALLOC on QNX. Yeah, this is the option 3 :)
Comment 18 Antonio Gomes 2012-06-13 12:30:15 PDT
> 
> #if OS(QNX) || PLATFORM(BLACKBERRY)
> ...
> #endif
> 
> ? What do you think?

only #if OS(QNX)

is enough.