Bug 51128 – [Symbian] OSAllocator implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Siddharth Mathur]

OSAllocatorSymbian.cpp needs to implement Symbian specific allocation (malloc style) as well as virtual memory management code (RChunk's commit() and decommit(). 
As far as specialized needs to JSC Heap go, ggaren suggests on IRC that we should strive to maximize use of the AlignedMemoryAllocator code, which already keeps track of bitmaps and commit/decommit of fixed sized blocks.

[Siddharth Mathur]

Created attachment 76682 [details]
*Half done* implementation for OSAllocatorSymbian.cpp. Also contains changes to remove Symbian-specific c'tors for PageReservation.

[Laszlo Gombos]

This patch should also reevaluate the following section in JavaScriptCore.pri:

symbian: {
    LIBS += -lhal
    # For hal.h
    INCLUDEPATH *= $$MW_LAYER_SYSTEMINCLUDE
}

If hal.h is no longer needed we should remove this section from JavaScriptCore.pri. If this section is still needed, we should move it to wtf.pri (where hal.h is used).

[Siddharth Mathur]

Created attachment 79609 [details]
Symbian implementation for OSAllocator

Some notes about the patch: 
- Most of the meat is in OSAllocatorSymbian.cpp
- Unlike Unix and Windows, we need context (in the form of an RChunk object) to do reserve() and commit(). Therefore, given the static nature of interfaces in OSAllocator.h and its consumer classes, I have used static variables for the chunk handles. 
- Based on the Usage enum and on the bytes requested, the commit or reserve request is routed to the appropriate backing chunk (via getBackingChunk() helper), or to the process' malloc/User::Alloc implementation (regular heap allocator) 
- I have tested that V8 benchmark test (version 5) runs to completion using QtTestBrowser on an N8 device.

[WebKit Review Bot]

Attachment 79609 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Siddharth Mathur]

Created attachment 79635 [details]
Fixed style issue, and updated Changelog to indicate that JSC JIT is currently not working and will be fixed in a later patch

[Siddharth Mathur]

Created attachment 79646 [details]
remove StackBounds.cpp from this patch

[Geoffrey Garen]

Comment on attachment 79646 [details]
remove StackBounds.cpp from this patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=79646&action=review

I think these comments are worth addressing.

I didn't review the Symbian allocation half of this patch.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:41
> +#define SYMBIAN_PAGESIZE(x)                         (HAL::Get(HALData::EMemoryPageSize, x));
> +#define SYMBIAN_ROUNDUPTOMULTIPLE(x, multipleof)    ((x + multipleof - 1) & ~(multipleof - 1))
> +// macro for checking if a given address is backed by the given (already initialized) chunk
> +#define IS_ADDRESS_FOR_CHUNK(address, chunk)  ((reinterpret_cast<TUint8*>(address) >= chunk->Base()) && \
> +        (reinterpret_cast<TUint8*>(address) < chunk->Base() + chunk->MaxSize())) 

These should be static inline functions, for type safety and debugging ease.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.h:30
> +#ifndef OSAllocatorSymbian_h
> +#define OSAllocatorSymbian_h

This file should be named WrappedChunk.h. WebKit naming guidelines name the file after the class.

> Source/JavaScriptCore/wtf/PageAllocationAligned.cpp:56
> +    size_t alignmentDelta(0);
> +#if !OS(SYMBIAN)    
> +    alignmentDelta = alignment - pageSize();
> +#endif

This seems wrong. How do you know that Symbian allocations will satisfy any arbitrary alignment requirement?

> Source/JavaScriptCore/wtf/VMTags.h:86
> +#define VM_TAG_FOR_TCMALLOC_MEMORY 1
> +#define VM_TAG_FOR_COLLECTOR_MEMORY 2
> +#define VM_TAG_FOR_EXECUTABLEALLOCATOR_MEMORY 3
> +#define VM_TAG_FOR_REGISTERFILE_MEMORY 4
> +#define VM_TAG_FOR_WEBCORE_PURGEABLE_MEMORY 5

It's a little weak for the Symbian allocator to be hard-wired for these specific kinds of allocations. I guess if we add more kinds of allocations in the future, Symbian will secretly inherit poor performance.

[Eric Seidel (no email)]

This seems like a bug gavin would like. :)

[Siddharth Mathur]

(In reply to comment #7)

Geoffrey, first thanks for your review. Yes, this is a pretty awkward implementation, so I will try my best to explain...
> 
> > Source/JavaScriptCore/wtf/PageAllocationAligned.cpp:56
> > +    size_t alignmentDelta(0);
> > +#if !OS(SYMBIAN)    
> > +    alignmentDelta = alignment - pageSize();
> > +#endif
> 
> This seems wrong. How do you know that Symbian allocations will satisfy any arbitrary alignment requirement?

I make an assumption here that PageAllocationAligned will always be used with allocation size == alignment requirement. The WrappedChunk class in OSAllocatorSymbian.cpp is written to meet this need very efficiently. Both collector blocks and the handle pool use case of PageAllocationAligned currently behave as such. Not future proof for sure, an ASSERT is surely required at a bare minimum. 
The main issue with Symbian OS is that I have to piggyback multiple commits (and 1 big reserve) on 1 RChunk object (bad karma to create chunks by the thousands in a process), the amount of housekeeping code that I would need to support arbitrarily sized blocks (even if pageSize multiples) would be a fair amount of effort, I think :) . Also there will be "holes" in the virtual address range which would be in uncommitted state (like in GC blocks use case) which then requires a search-next-available-region-of-size-X logic to be implemented. 

I could conceivably have created a new RChunk/WrappedChunk for each new 'size'  seen for usage type OSAllocator::JSGCHeapPages, and appended them to a dynamic array. That would make it keep working when something more than GC blocks and handle pool are added to the WebKit feature set. But, the downside is that I would have to a priori guesstimate what the max reservation size needs (see CreateDisconnectedLocal()'s third argument) to be for that new chunk. I can't create do a 1-1 mapping between an incoming reserve request and a (new) chunk. 

Any better ideas or clarifications are sincerely appreciated. 

> 
> > Source/JavaScriptCore/wtf/VMTags.h:86
> > +#define VM_TAG_FOR_TCMALLOC_MEMORY 1
> > +#define VM_TAG_FOR_COLLECTOR_MEMORY 2
> > +#define VM_TAG_FOR_EXECUTABLEALLOCATOR_MEMORY 3
> > +#define VM_TAG_FOR_REGISTERFILE_MEMORY 4
> > +#define VM_TAG_FOR_WEBCORE_PURGEABLE_MEMORY 5
> 
> It's a little weak for the Symbian allocator to be hard-wired for these specific kinds of allocations. I guess if we add more kinds of allocations in the future, Symbian will secretly inherit poor performance.

I fixed these defines because they were initializing the Usage enum which I use extensively. I completely agree that my supporting only a few types of these enums efficiently is not sustainable from a performance perspective as WebKit evolves . But I will appreciate any suggestion on how to make is better. I will of course be maintaining and improving this implementation over time. 

PS: 
Types of chunks in Symbian OS: http://tinyurl.com/49l3ao9
RChunk API doc: http://tinyurl.com/463aweb
UserHeap for helping with alignment (efficient only for small alignment values) : http://tinyurl.com/4hetjbz

[Siddharth Mathur]

Created attachment 79924 [details]
Converted macros into static inline functions and renamed new .h to WrappedChunkSymbian.h

[Geoffrey Garen]

(In reply to comment #10)
> I make an assumption here that PageAllocationAligned will always be used with allocation size == alignment requirement.

OK, that assumption is not valid. The reason I added the PageAllocationAligned was to support allocations where size and alignment may not be equal.

Darwin's allocator can match any alignment requirement, which is why it has a special code path. If you want to give Symbian a special code path that takes advantage of the minimum alignment guaranteed by the underlying allocator, that's fine. But clients should be able to change the sizes and alignments they request without having to write special code to work around Symbian ASSERTs or crashes.

> the amount of housekeeping code that I would need to support arbitrarily sized blocks (even if pageSize multiples) would be a fair amount of effort, I think :) .

OK, but the point of PageAllocation is to support arbitrarily sized blocks, so it sounds like you have some work to do :).

> I fixed these defines because they were initializing the Usage enum which I use extensively. I completely agree that my supporting only a few types of these enums efficiently is not sustainable from a performance perspective as WebKit evolves.

I think a better solution overall would build a slab allocator on top of large RChunk blocks. However, it's OK by me if the Symbian allocator performs poorly in a subtle way, as long as it doesn't interfere with general WebKit development.

[Siddharth Mathur]

Comment on attachment 79924 [details]
Converted macros into static inline functions and renamed new .h to WrappedChunkSymbian.h 

Evaluating another algorithm, so canceling review for now ;)

[Siddharth Mathur]

Created attachment 80211 [details]
New algorithm

Substantially redone implementation that doesn't care for Usage type and can flexibly handle any future uses of the OSAllocator interface

[Siddharth Mathur]

Created attachment 80213 [details]
New algo take 2

The PageSizedAllocatorSymbian class handles the job of using one Symbian OS chunk (with a large up-front address space reservation) and a bitmap (one bit per page-sized block of the address space) to handle all of OSAllocator needs in a flexible manner. 
Comments starting Line 126 of OSAllocatorSymbian.cpp explain the logic in more detail. 
(Look ma, no OSAllocator::Usage!)

[Laszlo Gombos]

Comment on attachment 80213 [details]
New algo take 2

View in context: https://bugs.webkit.org/attachment.cgi?id=80213&action=review

Overall the algorithm looks good to me but I think this needs an other rounds of reviews.

> Source/JavaScriptCore/wtf/OSAllocator.h:81
> +    // These OSes prefer an explicit decommit before a release

I find this comment confusing here. The reason to take this code path seems to be different on WINCE and on Symbian. I would suggest to have different explanation for WINCE and SYMBIAN. For Symbian it seems that taking this code path makes the allocator implementation more convenient.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:1
> - * Copyright (C) 2010 Apple Inc. All rights reserved

I the Apple Copyright should stay.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:44
> +static void* makeNewCodeChunk(size_t bytes) 

I would use allocateCodeChunk here as a function name.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:56
> +static bool freeExistingCodeChunk(void *address) 

I would use deallocateCodeChunk here as the function name. Also it should be "void* address" instead - this coding style guideline is violated several times in the patch.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:131
> +// - The entire reservation reserve is logically divided into pageSized blocks (4K on Symbian) 

Let's be consistent about the pageSize - make it a const 4Kb (and do not use HAL) or do not state definitely that the pageSize size is 4 Kb (and use HAL).

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:142
> +    // Initialize chunk .. 

Seems a useless comment.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:143
> +    RChunk c; 

Spell c out to chunk.

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:154
> +    const TUint32 bitsPerWord = 8*sizeof(TUint32); 

Missing spaces - should be " * "

> Source/JavaScriptCore/wtf/OSAllocatorSymbian.cpp:205
> +    __ASSERT_ALWAYS(m_map.get(idx+nPages-1), User::Panic(_L("OSAllocator3"), idx+nPages-1));

If you intend to keep these debug messages here than you should have more meaningful log entries.

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:33
> +#include <hal.h>

Perhaps it is enough to include hal.h in OSAllocatorSymbian.cpp and we do not need it in the header file.

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:74
> +    void destroy() 

Do we really need this function ? Seems it is only called from the destructor.

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:83
> +class PageSizedAllocatorSymbian {

I would prefer PageAllocatorSymbian.

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:89
> +    void *parkNextAvailableBlock(size_t bytes);

I think we should find a better verb here instead of park - can we just stick to reserve ?

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:94
> +    bool ownsAddress(const void* address) const; 

contains() sounds better and inline with for example PageBlock.h.

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:100
> +    struct {

Can we share some code with WTF::Bitmap here (and use less Symbian specific code) ?

> Source/JavaScriptCore/wtf/PageSizedAllocatorSymbian.h:101
> +        TUint32 *bits; // array of bit flags 

I think we should keep this on the stack and not on the heap - e.g. WTF::Bitmap<bitmapSize> m_bitmap;

[Siddharth Mathur]

Created attachment 82175 [details]
Updatd patch with review comments incorporated

This round has issues fixed from previous review, and utilizes Bitmap in wtf/Bitmap.h rather than rolling my own. 
I think the overall algorithm is quite sound, and I have done multiple runs on Sunspider and V8 with the JSC interpreter (not JIT), and things look solid. 

Known issues: 
- JIT seems to have pre-existing issues on Symbian (will fix as seperate bug), so all tests done  with interpreter. 
- No thread safety just yet because it's not important for Symbian port at the moment. 
- The implementation of Bitmap::findRunOfZeros() could be made faster/smarter.

[Siddharth Mathur]

Created attachment 82999 [details]
Rebaselined patch against ToT.

[Siddharth Mathur]

Created attachment 83011 [details]
Fixed comments, better class and member names, minor indent fix

[Siddharth Mathur]

Created attachment 83016 [details]
Indent fix (funny that style-checker doesn't catch it)

[Laszlo Gombos]

Comment on attachment 83016 [details]
Indent fix (funny that style-checker doesn't catch it) 

r=me.

As a follow-up Bitmap<size>::findRunOfZeros() can be optimized.

[WebKit Commit Bot]

Comment on attachment 83016 [details]
Indent fix (funny that style-checker doesn't catch it) 

Clearing flags on attachment: 83016

Committed r79126: <http://trac.webkit.org/changeset/79126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.