[PATCH] D42030: [WebAssembly] Define __heap_base global

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Writer.cpp:38
 static constexpr int kStackAlignment = 16;
+static constexpr int kHeapAlignment = 4096;
 
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > dschuff wrote:
> > > > Why 4096? Since wasm pages are 16k, is there some particular reason for it to be this as opposed to something else?
> > > No really any good reason...  should we just make it 16-byte aligned like the stack?  In which case there is not need to do any more alignedment since the heap will follow the stack region which is already 16-byte aligned.
> > > 
> > > Incidentally our musl limits.h currently defines PAGE_SIZE to 4k.. should we change it to 16k?
> > (I think Wasm pages are actually 64KiB: https://webassembly.github.io/spec/core/exec/runtime.html#page-size)
> > 
> > I //wouldn't// change the Musl PAGE_SIZE to match. (I looked into this when doing my Musl port, and Rich Felker added this as a review comment.) PAGE_SIZE doesn't have to match anything the CPU does, what it really has to match is what mmap does. So whatever alignment your mmap (and hence malloc) perform should be exposed via PAGE_SIZE. In my Musl, I chose 32KiB for the page size, and mmap works in those units, so that malloc rounds up "large" allocations (>100KiB) to the nearest libc page. It doesn't matter that grow_memory/current_memory Wasm instructions work in 64KiB units, the mmap implementation does the appropriate translation.
> I discusses with with @dschuff yesterday.   We decide tt makes sense to page size to be consistent with the level of granularity of the underlying memory system.   There is no reason for mmap or brk to operate on anything smaller than 64k chunks since you we can't allocate in smaller chunks.  i.e.  it makes sense to have the page size match.   Perhaps there is a reason I'm missing but I can't see any benefit in having two different concepts of page size.
I think it does make sense - the Wasm page size isn't exposed by any libc API, and there's no reason I can see why they _should_ be linked (other than, as you say, to avoid any confusion for the libc maintainers themselves).

mmap //can// allocate memory in smaller chunks than 64KiB - it can allocate chunks as small as you want, according to whatever allocation algorithm you've chosen. In Wasm, mmap will be built on top of `__builtin_wasm_grow_memory`, ie libc will be parcelling up the brk space into mmap-ed regions. It can parcel up the process space however it chooses. mmap needs to have some allocation mechanism in order for the interface to be implemented using sbrk.

In my Musl port, mmap has an 8K bitmap of all the possible pages (8K = 65536 bits) which it uses to "map" or "unmap" pages, expanding the brk as needed to create space for handing out more pages.

I went with 32KiB pages, rather than 64KiB, to reduce the amount of space wastage. With 64KiB pages, you'd only need 4K for the bitmap, but you'd waste up to 63KiB of space, which is decent fraction if you're doing ~150KiB allocations (for example). At 32KiB, the mmap space wastage is reduced from 33% to 20% for a single 129KiB allocation, ie the space you're wasting with mmap's bitmap is reclaimed immediately the first time you do a "medium-sized" allocation.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42030