[webkit-reviews] review denied: [Bug 24540] Fix for missing mmap features in Symbian : [Attachment 28716] Updated patch for bug 24540
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 20 01:18:11 PDT 2009
Mark Rowe (bdash) <mrowe at apple.com> has denied Norbert Leser
<norbert.leser at nokia.com>'s request for review:
Bug 24540: Fix for missing mmap features in Symbian
https://bugs.webkit.org/show_bug.cgi?id=24540
Attachment 28716: Updated patch for bug 24540
https://bugs.webkit.org/attachment.cgi?id=28716&action=review
------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
I'm going to say r- again as the patch looks like it won't compile, and has a
number of more minor issues. I'd also prefer that the change were less
invasive / better factored so the #if PLATFORM(SYMBIAN) code can be kept to a
minimum, or at least following our typical pattern of putting such
platform-specific code in separate implementation files. WebCore has better
examples of this practice than JavaScriptCore at present, but I think it would
be good to apply here too.
> @@ -124,7 +128,28 @@
> , m_globalObject(0)
> {
> size_t bufferLength = (capacity + maxGlobals) *
sizeof(Register);
> -#if HAVE(MMAP)
> +#if PLATFORM(SYMBIAN)
> + // Symbian's OpenC doesn't support MAP_ANON for mmap.
> + // Need to hack with fastMalloc.
> + // We may not really need memory alignment on page boundary
(4k),
> + // but it's safe this way, with very little overhead.
> + static size_t pagesize = getpagesize();
> + m_bufferSymbian = fastMalloc(bufferLength + pagesize);
> +
> + // Align memory at allocationSize boundary
> + uintptr_t ptr = reinterpret_cast<uintptr_t>(m_bufferSymbian);
> + size_t adjust = 0;
> + if ((ptr & (pagesize - 1)) != 0) {
> + adjust = pagesize - (ptr & (pagesize - 1));
> + }
> +
> + if (!m_bufferSymbian || adjust > pagesize || adjust < 0) {
> + fprintf(stderr, "Could not allocate register file: %d\n",
errno);
> + CRASH();
> + }
> + ptr += adjust;
> + m_buffer = reinterpret_cast<Register*>(ptr);
> +#elif HAVE(MMAP)
Can we factor this in a way that doesn't require putting 20 lines of
Symbian-specific code in this method? Putting code related to such a niche
platform first in the function is also a little obnoxious.
> m_buffer = static_cast<Register*>(mmap(0, bufferLength,
PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0));
> if (m_buffer == MAP_FAILED) {
> fprintf(stderr, "Could not allocate register file: %d\n",
errno);
> @@ -207,6 +232,9 @@
> #if HAVE(VIRTUALALLOC)
> Register* m_maxCommitted;
> #endif
> +#if PLATFORM(SYMBIAN)
> + void* m_bufferSymbian;
> +#endif
This variable name doesn't describe what the variable represents. The fact it
is Symbian-specific is obvious from the fact it's declared in PLATFORM(SYMBIAN)
block, so doesn't need to be mentioned in the name.
> +#include "config.h"
> +
> +#include "ExecutableAllocator.h"
There shouldn't be a blank line between these two #include's.
> +
> +#if ENABLE(ASSEMBLER)
> +
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +namespace JSC {
> +
> +void ExecutableAllocator::intializePageSize()
> +{
> + ExecutableAllocator::pageSize = getpagesize();
> +}
> +
> +ExecutablePool::Allocation ExecutablePool::systemAlloc(size_t n)
> +{
> + // Symbian's OpenC doesn't support MAP_ANON for mmap.
> + // Need to hack with fastMalloc.
> + ExecutablePool::Allocation alloc =
{reinterpret_cast<char*>(fastMalloc(n)), n};
> + return alloc;
> +}
> +
> +void ExecutablePool::systemRelease(const ExecutablePool::Allocation& alloc)
> +{
> + fastFree(reinterpret_cast<void*>alloc.pages);
> +}
> +
> +}
> +
> +#endif // HAVE(ASSEMBLER)
Does this code even compile? The systemRelease method seems particular
suspicious.
> @@ -87,6 +92,11 @@
> // a PIC branch in Mach-O binaries, see <rdar://problem/5971391>.
> #define MIN_ARRAY_SIZE (static_cast<size_t>(14))
>
> +#if PLATFORM(SYMBIAN)
> + const size_t MAX_NUM_BLOCKS = 128; // Max size of collector heap set to
8 MB
> + static RHeap* userChunk = NULL;
> +#endif
> +
We don't typically indent in this manner.
> static void freeHeap(CollectorHeap*);
>
> #if ENABLE(JSC_MULTIPLE_THREADS)
> @@ -128,6 +138,27 @@
> {
> ASSERT(globalData);
>
> +#if PLATFORM(SYMBIAN)
> + // Symbian OpenC supports mmap but currently not the MAP_ANON flag.
> + // Using fastMalloc() does not properly align blocks on 64k boundaries
> + // and previous implementation was flawed/incomplete.
> + // UserHeap::ChunkHeap allows allocation of continuous memory and
specification
> + // of alignment value for (symbian) cells within that heap.
> + //
> + // Clarification and mapping of terminology:
> + // RHeap (created by UserHeap::ChunkHeap below) is continuos memory
chunk,
> + // which can dynamically grow up to 8 MB,
> + // that holds all CollectorBlocks of this session (static).
> + // Each symbian cell within RHeap maps to a 64kb aligned CollectorBlock.
> + // JSCell objects are maintained as usual within CollectorBlocks.
> + if (NULL == userChunk) {
> + userChunk = UserHeap::ChunkHeap (NULL, 0, MAX_NUM_BLOCKS *
BLOCK_SIZE, BLOCK_SIZE, BLOCK_SIZE);
> + if (NULL == userChunk) {
> + CRASH();
> + }
> + }
> +#endif // PLATFORM(SYMBIAN)
> +
> memset(&primaryHeap, 0, sizeof(CollectorHeap));
> memset(&numberHeap, 0, sizeof(CollectorHeap));
> }
> @@ -184,10 +215,18 @@
Besides the coding style issues here (explicit comparisons with NULL, braces
around one-line if statements, use of NULL where we prefer 0, and whitespace
between function name and arguments), it's rather horrible to have a huge chunk
of #if'd code in this otherwise platform-independent method. It would be
preferable to factor this in some manner that avoids this. The use of a static
in this code doesn't seem thread-safe either.
> vm_address_t address = 0;
> // FIXME: tag the region as a JavaScriptCore heap when we get a
registered VM tag: <rdar://problem/6054788>.
> vm_map(current_task(), &address, BLOCK_SIZE, BLOCK_OFFSET_MASK,
VM_FLAGS_ANYWHERE, MEMORY_OBJECT_NULL, 0, FALSE, VM_PROT_DEFAULT,
VM_PROT_DEFAULT, VM_INHERIT_DEFAULT);
> +
> #elif PLATFORM(SYMBIAN)
> - // no memory map in symbian, need to hack with fastMalloc
> - void* address = fastMalloc(BLOCK_SIZE);
> +
> + // Allocate a 64 kb aligned CollectorBlock
> + unsigned char* mask = (unsigned char*)userChunk->Alloc(BLOCK_SIZE);
> + if (NULL == mask) {
> + CRASH();
> + }
> + uintptr_t address = reinterpret_cast<uintptr_t>(mask);
> +
> memset(reinterpret_cast<void*>(address), 0, BLOCK_SIZE);
> +
> #elif PLATFORM(WIN_OS)
> // windows virtual address granularity is naturally 64k
> LPVOID address = VirtualAlloc(NULL, BLOCK_SIZE, MEM_COMMIT |
MEM_RESERVE, PAGE_READWRITE);
> @@ -231,7 +270,7 @@
> #if PLATFORM(DARWIN)
> vm_deallocate(current_task(), reinterpret_cast<vm_address_t>(block),
BLOCK_SIZE);
> #elif PLATFORM(SYMBIAN)
> - fastFree(block);
> + userChunk->Free(reinterpret_cast<TAny *>(block));
> #elif PLATFORM(WIN_OS)
> VirtualFree(block, 0, MEM_RELEASE);
> #elif HAVE(POSIX_MEMALIGN)
> @@ -395,6 +434,16 @@
There are a few coding style issues here. The use of C-style casts, braces
around one-line if statements, explicit comparisons with NULL and whitespace
before the * in type names are ones that jump out at me.
> +#elif PLATFORM(SYMBIAN)
> + // Needs to be positioned before other platforms to avoid potential
ambiguities
> + static void* stackBase = 0;
> + if (stackBase == 0) {
> + TThreadStackInfo info;
> + RThread thread;
> + thread.StackInfo(info);
> + stackBase = (void*)info.iBase;
> + }
> + return (void*)stackBase;
> #elif PLATFORM(WIN_OS) && PLATFORM(X86) && COMPILER(MSVC)
> // offset 0x18 from the FS segment register gives a pointer to
> // the thread information block for the current thread
> @@ -446,15 +495,6 @@
> stackThread = thread;
> }
> return static_cast<char*>(stackBase) + stackSize;
> -#elif PLATFORM(SYMBIAN)
> - static void* stackBase = 0;
> - if (stackBase == 0) {
> - TThreadStackInfo info;
> - RThread thread;
> - thread.StackInfo(info);
> - stackBase = (void*)info.iBase;
> - }
> - return (void*)stackBase;
> #else
> #error Need a way to get the stack base on this platform
> #endif
Why does this code need to move? The ChangeLog doesn't mention it at all.
More information about the webkit-reviews
mailing list