Bug 29256 – Use MemoryManager to gather platform-specific low level memory requests

• Component: Platform
• Status: RESOLVED WONTFIX

[Yong Li]

There are a few places in JSC where low-level memory allocation is required:

1. Collector.cpp: allocating 256k (previously 64k) JS blocks.
2. RegisterFile.h, cpp: reserving a big chunk of virtual memory and committing gradually.
3. MarkStack*.cpp: allocating/increasing aligned memory.
.....

Introducing an abstract layer MemoryManager allows better low-level memory management and customized implementations.

Patch is coming soon.

[Yong Li]

Created attachment 39598 [details]
the patch

The patch. 

MemoryManagerPseudo.cpp : Implementation with malloc() and free()
MemoryManagerWin.cpp: Simple solution for WIN32

[Yong Li]

Created attachment 39599 [details]
the patch

[Yong Li]

Created attachment 39600 [details]
the patch

[Geoffrey Garen]

What's the goal of this patch? JavaScriptCore already supports malloc/free and Windows.

I disagree with the approach here.

1. There's a layering problem. An "abstract" memory allocator should be agnostic about the kinds of allocations its clients will make. But this allocator has functions like allocateJSBlock and reserveJSRegisterFile, tying it to its clients. This seems like a confused and unscalable design.

2. You haven't ported any existing platforms to the abstract interface, so you've just added yet another way to allocate memory, complicating the design.

[Yong Li]

(In reply to comment #4)
> What's the goal of this patch? JavaScriptCore already supports malloc/free and
> Windows.
> 

One goal is to finally remove those #if PLATFORM(...), #if HAVE(MMAP) and #if HAVE(VIRTUAL_ALLOC) things from other source files. Like CurrentTime.cpp, we use WTF::currentTime() everywhere, so we don't have to put the following code everywhere.

 #if PLATFORM(POSIX)
 ... gettimeofday
#elif PLATFORM(WIN)
 ... GetSystemTime
...

Another goal is try to gather all low-level memory requests together, so that at runtime we can have a better view of the memory usage, and can do something special when it fails to allocate more, or when memory is under pressure.

I also have an idea about lazy memory freeing for OwnPtr and RefPtr. That could boost performance. To make it work perfectly, we need to get notified upon memory allocation failure. in MSVC, there's a _set_new_handler() that can be used to do cleanup when malloc or new fails. It should be doable to add such a callback to fastMalloc implemenation. So at the time we cannot allocate more memory, we have to free those delayed memory blocks immediately.

All of such things need a memory manager that takes control of overall memory usage.

Another benefit of memory manager is that it makes customization easier. People don't have to modify those JavaScriptCore source code for each special port.


> 1. There's a layering problem. An "abstract" memory allocator should be
> agnostic about the kinds of allocations its clients will make. But this
> allocator has functions like allocateJSBlock and reserveJSRegisterFile, tying
> it to its clients. This seems like a confused and unscalable design.
>

I was using more common functions like allocateVirtualMemory. But then it could be hard to optimize. For example, Collector.cpp allocates 256K JS blocks. If MemoryManager knows about it's for JS, then it may allocate a big chunk of memory (2M for example), and cut them into 256k blocks as a pool for JS.
 
> 2. You haven't ported any existing platforms to the abstract interface, so
> you've just added yet another way to allocate memory, complicating the design.

I provide a MemoryManagerWin.cpp. It compiles, but it includes runtime/Collector.h. I think it's up to win32 developer to decide to use or not use it.

I also provide a MemoryManagerPseudo.cpp, which provides a simple solution for RegsiterFie, MarkStack and JS blocks, without real virtual memory.

[Geoffrey Garen]

> One goal is to finally remove those #if PLATFORM(...), #if HAVE(MMAP) and #if
> HAVE(VIRTUAL_ALLOC) things from other source files. Like CurrentTime.cpp, we
> use WTF::currentTime() everywhere, so we don't have to put the following code
> everywhere.

I can see some value there. But, like I said, your patch makes the problem you're trying to solve worse, by adding another #if without removing any.

> Another goal is try to gather all low-level memory requests together, so that
> at runtime we can have a better view of the memory usage, and can do something
> special when it fails to allocate more, or when memory is under pressure.

What would you do when memory was under pressure?

> I also have an idea about lazy memory freeing for OwnPtr and RefPtr. That could
> boost performance. To make it work perfectly, we need to get notified upon
> memory allocation failure. in MSVC, there's a _set_new_handler() that can be
> used to do cleanup when malloc or new fails. It should be doable to add such a
> callback to fastMalloc implemenation. So at the time we cannot allocate more
> memory, we have to free those delayed memory blocks immediately.

I don't think that's a good direction to go in. On most systems, it will cause huge page fault and paging overhead.

> > 1. There's a layering problem. An "abstract" memory allocator should be
> > agnostic about the kinds of allocations its clients will make. But this
> > allocator has functions like allocateJSBlock and reserveJSRegisterFile, tying
> > it to its clients. This seems like a confused and unscalable design.
> >
> 
> I was using more common functions like allocateVirtualMemory. But then it could
> be hard to optimize. For example, Collector.cpp allocates 256K JS blocks. If
> MemoryManager knows about it's for JS, then it may allocate a big chunk of
> memory (2M for example), and cut them into 256k blocks as a pool for JS.

If you want to optimize for clients that expect to make more calls for allocations of the same sizes, just add an explicit API for pools.

I don't think a pool would be a good idea in the case of collector blocks, because a block is already a pool, and the collector already manages a pool of blocks, based on information that it has and the VM allocator does not have, like total GC heap size.

> > 2. You haven't ported any existing platforms to the abstract interface, so
> > you've just added yet another way to allocate memory, complicating the design.
> 
> I provide a MemoryManagerWin.cpp. It compiles, but it includes
> runtime/Collector.h. I think it's up to win32 developer to decide to use or not
> use it.
> 
> I also provide a MemoryManagerPseudo.cpp, which provides a simple solution for
> RegsiterFie, MarkStack and JS blocks, without real virtual memory.

OK, but like I said before, you've increased complexity rather than decreasing it.

[Yong Li]

(In reply to comment #6)
> 
> I can see some value there. But, like I said, your patch makes the problem
> you're trying to solve worse, by adding another #if without removing any.
> 

I can #define HAVE_MEMORYMANAGER 1 and make it build with MemoryManagerWin.cpp. In this case, I can remove #if PLATFORM(WIN_OS) (allocating js block in collector.cpp), and #if HAVE(VIRTUAL_ALLOC) in RegisterFile and MarkStackWin.cpp. But I'm not sure if adding ../runtime/ to the INCLUDE of wtf project file is acceptable for win32 developers.

It shouldn't be difficult to implement a MemoryMangerPosix for the ports that uses mmap(). But I don't have a build environment for that.

[George Staikos]

Geoff, how would you like to see this done?

[Yong Li]

closed by me. feel free to re-open it if you think necessary