[webkit-reviews] review granted: [Bug 34350] [Symbian] More efficient aligned memory allocation for JSC Collector : [Attachment 50966] Round 2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 18 09:29:35 PDT 2010
Laszlo Gombos <laszlo.1.gombos at nokia.com> has granted Siddharth Mathur
<siddharth.mathur at nokia.com>'s request for review:
Bug 34350: [Symbian] More efficient aligned memory allocation for JSC Collector
https://bugs.webkit.org/show_bug.cgi?id=34350
Attachment 50966: Round 2
https://bugs.webkit.org/attachment.cgi?id=50966&action=review
------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
Great patch Siddharth !
The direction is good to optimize the Collector allocator to Symbian and move
some Symbian logic out from Collector.cpp and isolate it into its own class.
Maybe BlockAllocatorSymbian (with capitalization) is a better name than
pagedallocator.h. It is good to carry the Platform name in the file name as
well, so avoid name collisions with same file names in different directories.
> Index: JavaScriptCore/runtime/Collector.cpp
> ===================================================================
> -#if OS(SYMBIAN)
> -const size_t MAX_NUM_BLOCKS = 256; // Max size of collector heap set to 16
MB
> -static RHeap* userChunk = 0;
> -#endif
> -#endif // OS(SYMBIAN)
> + // A convenience class to allocate aligned blocks efficiently
> + m_blockallocator = new WTF::AlignedBlockAllocator(RESERVATION,
BLOCK_SIZE);
> +#endif
Moving the chunk from static to a member seems like a good idea.
> #elif OS(SYMBIAN)
> - static void* stackBase = 0;
> - if (stackBase == 0) {
> - TThreadStackInfo info;
> - RThread thread;
> - thread.StackInfo(info);
> - stackBase = (void*)info.iBase;
> - }
> - return (void*)stackBase;
> + TThreadStackInfo info;
> + RThread thread;
> + thread.StackInfo(info);
> + return reinterpret_cast<void*>(info.iBase);
This change does not seems to be strictly related to the rest of the patch,
caching the stackBase might be a good idea. I would propose to take this change
out from this patch.
> Index: JavaScriptCore/runtime/Collector.h
> ===================================================================
> -#if OS(WINCE) || OS(SYMBIAN)
> +#if OS(WINCE)
> const size_t BLOCK_SIZE = 64 * 1024; // 64k
> +#elif OS(SYMBIAN)
> + const size_t BLOCK_SIZE = 64 * 1024; // 64k
> + // Virtual address range to reserve
> + const size_t RESERVATION = JSCCOLLECTOR_VIRTUALMEM_RESERVATION;
> #else
> const size_t BLOCK_SIZE = 64 * 4096; // 256k
> #endif
Can we move RESERVATION = JSCCOLLECTOR_VIRTUALMEM_RESERVATION part into
Collector.cpp ?
> Index: JavaScriptCore/wtf/symbian/pagedallocator.cpp
> ===================================================================
I think it would be desired to remove Qt dependency from the Allocator. We
should either use the WebKit logging mechanism or remove tracing.
> --- JavaScriptCore/wtf/symbian/pagedallocator.cpp (revision 0)
> +++ JavaScriptCore/wtf/symbian/pagedallocator.cpp (revision 0)
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + * its contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "pagedallocator.h"
> +#include <QDebug>
> +
> +namespace WTF {
> +
> +/** Efficiently allocates blocks of size blockSize with blockSize alignment.
> + * Primarly designed for JSC Collector's needs.
> + * Not thread-safe.
> + */
> +AlignedBlockAllocator::AlignedBlockAllocator(TUint32 reservationSize,
TUint32 blockSize )
> + : m_reservation(reservationSize),
> + m_blockSize(blockSize)
> +{
> +
> + // Get system's page size value.
> + SYMBIAN_PAGESIZE(m_pageSize);
> + // We only accept multiples of system page size for both initial
reservation and the alignment/block size
> + m_reservation = SYMBIAN_ROUNDUPTOMULTIPLE(m_reservation, m_pageSize);
> + __ASSERT_ALWAYS(SYMBIAN_ROUNDUPTOMULTIPLE(m_blockSize, m_pageSize),
User::Panic(_L("AlignedBlockAllocator1"), KErrArgument));
> +
> + // Calculate max. bit flags we need to carve a reservationSize range
into blockSize-sized blocks
> + m_map.numBits = m_reservation / m_blockSize;
> + const TUint32 bitsPerWord = 8*sizeof(TUint32);
> + const TUint32 numWords = (m_map.numBits + bitsPerWord -1) /
bitsPerWord;
> + qDebug("AlignedBlockAllocator: %d bit flags (%d integers) to represent
%d MB reservation with %d block size",
> + m_map.numBits, numWords, m_reservation / (1024*1024),
m_blockSize);
> +
> + m_map.bits = new TUint32[numWords];
> + __ASSERT_ALWAYS(m_map.bits, User::Panic(_L("AlignedBlockAllocator2"),
KErrNoMemory));
> + m_map.clearAll();
> +
> + // Open a Symbian RChunk, and reserve requested virtual address range
> + // Any thread in this process can operate this rchunk due to
EOwnerProcess access rights.
> + TInt ret = m_chunk.CreateDisconnectedLocal(0 , 0, (TInt)m_reservation ,
EOwnerProcess);
> + if (ret != KErrNone)
> + User::Panic(_L("AlignedBlockAllocator3"), ret);
> +
> + // This is the offset to m_chunk.Base() required to make it
m_blockSize-aligned
> + m_offset = SYMBIAN_ROUNDUPTOMULTIPLE(TUint32(m_chunk.Base()),
m_blockSize) - TUint(m_chunk.Base());
> + qDebug("AlignedBlockAllocator: chunk handle = %d, m_chunk.Base() = %x,
m_offset = %d ", m_chunk.Handle(), m_chunk.Base(), m_offset);
> +
> +}
> +
> +void* AlignedBlockAllocator::alloc()
> +{
> +
> + TInt freeRam = 0;
> + void* address = 0;
> +
> + // Look up first free slot in bit map
> + const TInt freeIdx = m_map.findFree();
> +
> + if (freeIdx < 0) {
> + // All bits are already set ("Committed"), so no more blocks
> + // can be carved out of the virtual address range.
> + SYMBIAN_FREERAM(freeRam);
> + qDebug("AlignedBlockAllocator::Alloc pseudo-OOM %d kB", freeRam /
1024);
> + return address;
For readability it might be better to explicitly return 0 here.
> + }
> +
> + qDebug("AlignedBlockAllocator: freeIdx %d, Committing at offset = %d",
freeIdx, m_offset + (m_blockSize * freeIdx));
> + TInt ret = m_chunk.Commit(m_offset + (m_blockSize * freeIdx),
m_blockSize);
> + if (ret != KErrNone) {
> + qDebug("AlignedBlockAllocator Commit: %d", ret);
> + return address;
Same here.
> + }
> +
> + // Updated bit to mark region as in use.
> + m_map.set(freeIdx);
> +
> + // Calculate address of committed region (block)
> + address = (void*)( (m_chunk.Base() + m_offset) + (TUint)(m_blockSize *
freeIdx) );
> + qDebug("AlignedBlockAllocator alloc: address = %x, m_chunk.Base() = %d,
m_offset = %d", address, m_chunk.Base(), m_offset);
> +
> + return address;
> +}
> +
> +void AlignedBlockAllocator::free(void* block)
> +{
> + // Calculate index of block to be freed
> + TInt idx = TUint(static_cast<TUint8*>(block) - m_chunk.Base() -
m_offset) / m_blockSize;
> + qDebug("AlignedBlockAllocator free: calculated idx %d for block %x using
m_chunk.Base() = %x, m_offset = %d", idx, static_cast<TUint8 *>(block),
m_chunk.Base(), m_offset);
> +
> + __ASSERT_DEBUG(idx >= 0 && idx < m_map.numBits,
User::Panic(_L("AlignedBlockAllocator4"), KErrCorrupt)); // valid index check
> + __ASSERT_DEBUG(m_map.get(idx), User::Panic(_L("AlignedBlockAllocator5"),
KErrCorrupt)); // in-use flag check
> +
> + // Return committed region to system RAM pool (the physical RAM becomes
usable by others)
> + TInt ret = m_chunk.Decommit(m_offset + m_blockSize * idx, m_blockSize);
> + if ( ret != KErrNone )
> + qDebug("AlignedBlockAllocator::Decommit %d", ret);
> +
> + // mark this available again
> + m_map.clear(idx);
> +}
> +
> +void AlignedBlockAllocator::destroy()
> +{
> + // release everything!
> + m_chunk.Decommit(0, m_chunk.MaxSize());
Maybe clear the m_map bitflag here as well.
> +}
> +
> +AlignedBlockAllocator::~AlignedBlockAllocator()
> +{
> + destroy();
> + m_chunk.Close();
> + delete [] m_map.bits;
> +}
> +
> +} // end of namespace
> +
> Index: JavaScriptCore/wtf/symbian/pagedallocator.h
> ===================================================================
> --- JavaScriptCore/wtf/symbian/pagedallocator.h (revision 0)
> +++ JavaScriptCore/wtf/symbian/pagedallocator.h (revision 0)
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + * its contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef pagedallocator_h
> +#define pagedallocator_h
> +
> +#include <e32cmn.h>
> +#include <e32std.h>
> +#include <hal.h>
> +
> +
> +#define SYMBIAN_PAGESIZE(x) (HAL::Get(HALData::EMemoryPageSize, x));
> +#define SYMBIAN_FREERAM(x) (HAL::Get(HALData::EMemoryRAMFree, x));
> +#define SYMBIAN_ROUNDUPTOMULTIPLE(x, multipleof) ( (x + multipleof - 1) &
~(multipleof - 1) )
> +
> +// Set sane defaults if -D<flagname=value> wasn't provided via compiler args
> +#ifndef JSCCOLLECTOR_VIRTUALMEM_RESERVATION
> +#if defined(__WINS__)
> + // Emulator has limited virtual addresses space
> + #define JSCCOLLECTOR_VIRTUALMEM_RESERVATION (4*1024*1024)
> +#else
> + // HW has plenty of virtual addresses
> + #define JSCCOLLECTOR_VIRTUALMEM_RESERVATION (64*1024*1024)
> +#endif
> +#endif
> +
> +namespace WTF {
> +
> +/**
> + * Allocates contiguous region of size blockSize with blockSize-aligned
address.
> + * blockSize must be a multiple of system page size (typically 4K on
Symbian/ARM)
> + *
> + * @param reservationSize Virtual address range to be reserved upon
creation of chunk (bytes).
> + * @param blockSize Size of a single allocation. Returned address will also
be blockSize-aligned.
> + */
> +class AlignedBlockAllocator {
> + public:
> + AlignedBlockAllocator(TUint32 reservationSize, TUint32 blockSize);
> + ~AlignedBlockAllocator();
> + void destroy();
> + void* alloc();
> + void free(void* data);
> +
> + private:
> + RChunk m_chunk; // Symbian chunk that lets us
reserve/commit/decommit
> + TUint m_offset; // offset of first committed region from base
> + TInt m_pageSize; // cached value of system page size, typically
4K on Symbian
> + TUint32 m_reservation;
> + TUint32 m_blockSize;
> +
> + // Tracks comitted/decommitted state of a blockSize region
> + struct {
> +
> + TUint32 *bits; // array of bit flags
> + TUint32 numBits; // number of regions to keep track of
> +
> + bool get(TUint32 n) const
> + {
> + return !!(bits[n >> 5] & (1 << (n & 0x1F)));
> + }
> +
> + void set(TUint32 n)
> + {
> + bits[n >> 5] |= (1 << (n & 0x1F));
> + }
> +
> + void clear(TUint32 n)
> + {
> + bits[n >> 5] &= ~(1 << (n & 0x1F));
> + }
> +
> + void clearAll()
> + {
> + for (TUint32 i = 0; i < numBits; i++)
> + clear(i);
> + }
> +
> + TInt findFree() const
> + {
> + for (TUint32 i = 0; i < numBits; i++) {
> + if (!get(i))
> + return i;
> + }
> + return -1;
> + }
I would prefer to have some of these functions implemented in a cpp file not in
a .h file, but I noticed Collector.h has a similar model. Up to you if you want
to change this or not.
> +
> + } m_map;
> +
> +};
> +
> +}
> +
> +#endif
> +
> +
The style bot also picked up a few minor issues.
I'm OK with this change if you successfully run jsc module test with this
change. Can you confirm ?
Also, do you have performance numbers (before and after) with this change ?
r- from me, just to see if some of the comments can be addressed, but the
approach and the algorithm looks very good.
More information about the webkit-reviews
mailing list