Bug 77013 – [BlackBerry] Implement OSAllocator::commit/decommit

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

[Yong Li]

BlackBerry port should also support decommitting virtual memory.

[Yong Li]

Created attachment 123947 [details]
the patch

[Rob Buis]

Comment on attachment 123947 [details]
the patch

LGTM.

[WebKit Review Bot]

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

[Yong Li]

(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.

[WebKit Review Bot]

Comment on attachment 123947 [details]
the patch

Clearing flags on attachment: 123947

Committed r105918: <http://trac.webkit.org/changeset/105918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

Actually we should implement this in another way

[Yong Li]

Created attachment 132622 [details]
new solution

[Rob Buis]

Comment on attachment 132622 [details]
new solution

LGTM.

[WebKit Review Bot]

Comment on attachment 132622 [details]
new solution

Clearing flags on attachment: 132622

Committed r111234: <http://trac.webkit.org/changeset/111234>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Milian Wolff]

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?

[Yong Li]

(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.

[Milian Wolff]

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?

[Yong Li]

(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.

[Milian Wolff]

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?

[Yong Li]

(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 :)

[Antonio Gomes]

> 
> #if OS(QNX) || PLATFORM(BLACKBERRY)
> ...
> #endif
> 
> ? What do you think?

only #if OS(QNX)

is enough.