[webkit-reviews] review denied: [Bug 51128] [Symbian] OSAllocator implementation : [Attachment 80213] New algo take 2

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Siddharth Mathur
<siddharth.mathur at nokia.com>'s request for review:
Bug 51128: [Symbian] OSAllocator implementation
https://bugs.webkit.org/show_bug.cgi?id=51128

Attachment 80213: New algo take 2
https://bugs.webkit.org/attachment.cgi?id=80213&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
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;