To be smart on RAM usage, RegisterFile should NOT allocate (commit) 4MB upfront, but use logic similar to the HAVE(VIRTUALALLOC) case. a) use RChunk.CreateLocalDisconnected() to reserve virtual address space in the c'tor. https://trac.webkit.org/browser/trunk/JavaScriptCore/interpreter/RegisterFile.h b) use RChunk.commit() to commit only what is needed in RegisterFile::grow() https://trac.webkit.org/changeset/42842/trunk/JavaScriptCore/interpreter/RegisterFile.cpp c) decommit using RChunk.decommit() what's not needed in RegisterFile::shrink() Note that there are some alignment requirements for allocated memory.
How required is it (i.e. does the current implementation cause out of memory on some configuration)?
Kent, High (but not top) priority. We are very frequently out of memory on Symbian, since the devices have ~128MB of RAM and only 50-60MB free after boot. Due to mid-market positioning of Symbian devices, there will always be new phones out with only 128MB of RAM (and no virtual memory of course). :) The user of custom memory allocators for Qt-Symbian apps helps a bit here, but only a few will be using it. In my patch for Bug 34350, I was initially attempting to make the logic support RegisterFile's needs too, but the code got complicated quickly, so I only addressed Collector's needs.
Created attachment 56375 [details] patch file -- first take This is my first attempt at a patch for this issue. There's still some debugging code in the patch which can and will be removed before the patch is landed. Mostly, I'd just appreciate some early feedback at the moment. I'm working on a suite of unit tests for the patch and will post that shortly, too.
Attachment 56375 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp:34: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp:105: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp:145: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] JavaScriptCore/interpreter/RegisterFile.h:173: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] JavaScriptCore/wtf/symbian/ContigBlockAllocator.h:29: #ifndef header guard has wrong style, please use: ContigBlockAllocator_h [build/header_guard] [5] JavaScriptCore/wtf/symbian/ContigBlockAllocator.h:55: One space before end of line comments [whitespace/comments] [5] JavaScriptCore/wtf/symbian/ContigBlockAllocator.h:56: One space before end of line comments [whitespace/comments] [5] JavaScriptCore/wtf/symbian/ContigBlockAllocator.h:57: One space before end of line comments [whitespace/comments] [5] JavaScriptCore/wtf/symbian/ContigBlockAllocator.h:58: One space before end of line comments [whitespace/comments] [5] JavaScriptCore/wtf/symbian/SymbianDefines.h:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. JavaScriptCore/wtf/symbian/SymbianDefines.h:29: #ifndef header guard has wrong style, please use: SymbianDefines_h [build/header_guard] [5] Total errors found: 51 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 56376 [details] patch -- first take, style fixed D'oh! OK, let's try this again with improved style.
Attachment 56376 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/interpreter/RegisterFile.h:173: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 56376 [details] patch -- first take, style fixed I don't think you should allocate m_blockAllocator using new. Who's a good person to review the symbian parts of this patch?
Geoffrey, thanks for the look I plan to review this the Symbian part of this.
Comment on attachment 56376 [details] patch -- first take, style fixed > Index: JavaScriptCore/JavaScriptCore.pro > =================================================================== > --- JavaScriptCore/JavaScriptCore.pro (revision 59669) > +++ JavaScriptCore/JavaScriptCore.pro (working copy) > @@ -214,6 +214,7 @@ SOURCES += \ > wtf/RandomNumber.cpp \ > wtf/RefCountedLeakCounter.cpp \ > wtf/symbian/BlockAllocatorSymbian.cpp \ > + wtf/symbian/ContigBlockAllocator.cpp \ May I suggest RegisterFileAllocatorSymbian.cpp ? Ditto for the header file. The grow/shrink logic is custom made for the register file, and besides Qt conventionally adds platform suffix at the end of the filename. This may also give space to future contributors/other ports to move out some of the current platform-specific code for mmap() and VirtualAlloc() into their own RegisterFileAllocator<Platform>.cpp. > Index: JavaScriptCore/interpreter/RegisterFile.cpp > =================================================================== > --- JavaScriptCore/interpreter/RegisterFile.cpp (revision 59669) > +++ JavaScriptCore/interpreter/RegisterFile.cpp (working copy) > @@ -42,6 +42,8 @@ RegisterFile::~RegisterFile() > VirtualFree(m_buffer, DWORD(m_commitEnd) - DWORD(m_buffer), MEM_DECOMMIT); > #endif > VirtualFree(m_buffer, 0, MEM_RELEASE); > +#elif PLATFORM(SYMBIAN) > + delete m_blockAllocator; m_registerFileAllocator ? Which is also not heap allocated as ggaren suggests. > #else > fastFree(m_buffer); > #endif > Index: JavaScriptCore/interpreter/RegisterFile.h > =================================================================== > --- JavaScriptCore/interpreter/RegisterFile.h (revision 59669) > +++ JavaScriptCore/interpreter/RegisterFile.h (working copy) > @@ -42,6 +42,10 @@ > #include <sys/mman.h> > #endif > > +#if PLATFORM(SYMBIAN) > +#include <wtf/symbian/ContigBlockAllocator.h> > +#endif > + Header file name change. > #if HAVE(VIRTUALALLOC) > Register* m_commitEnd; > #endif > @@ -159,7 +168,12 @@ namespace JSC { > }; > > // FIXME: Add a generic getpagesize() to WTF, then move this function to WTF as well. > + // This is still a hack that should be fixed later. We know that a Symbian page size is 4K. > +#if PLATFORM(SYMBIAN) > + inline bool isPageAligned(size_t size) { return size != 0 && size % (4 * 1024) == 0; } > +#else > inline bool isPageAligned(size_t size) { return size != 0 && size % (8 * 1024) == 0; } > +#endif > > inline RegisterFile::RegisterFile(size_t capacity, size_t maxGlobals) > : m_numGlobals(0) > @@ -205,6 +219,14 @@ namespace JSC { > CRASH(); > } > m_commitEnd = reinterpret_cast<Register*>(reinterpret_cast<char*>(m_buffer) + committedSize); > + #elif PLATFORM(SYMBIAN) > + // A convenience class to allocate contiguous blocks efficiently > + m_blockAllocator = new WTF::ContigBlockAllocator(bufferLength, 64 * 1024); > + m_buffer = (Register*)(m_blockAllocator->buffer()); > + Regarding the second argument to the c'tor, it seems to be the threshold below which we don't commit or decommit (i.e the internal pool of committed memory). What are the typical shrink/grow delta you see requested in typical web browsing? Perhaps ggaren can help here . The c'tor arg should also be #defined and parked inside wtf/symbian/youheaderfile.h > + // start by committing enough space to hold maxGlobals > + void* newEnd = (void*)((int)m_buffer + (maxGlobals * sizeof(Register))); > + m_blockAllocator->grow(newEnd); > #else static_cast<void*>m_buffer + (maxGlobals * sizeof(Register)) ? > /* > * If neither MMAP nor VIRTUALALLOC are available - use fastMalloc instead. > @@ -227,8 +249,12 @@ namespace JSC { > if (newEnd >= m_end) > return; > m_end = newEnd; > - if (m_end == m_start && (m_maxUsed - m_start) > maxExcessCapacity) > + if (m_end == m_start && (m_maxUsed - m_start) > maxExcessCapacity) { > +#if PLATFORM(SYMBIAN) > + m_blockAllocator->shrink(newEnd); > +#endif > releaseExcessCapacity(); > + } > } > I presume we can't use the (m_max - m_start)* sizeof(Register) logic in releaseExcessCapacity() to decommit ? Sorry if I missed something here. > inline bool RegisterFile::grow(Register* newEnd) > @@ -254,6 +280,10 @@ namespace JSC { > } > #endif > > +#if PLATFORM(SYMBIAN) > + m_blockAllocator->grow((void*)newEnd); > +#endif > + > if (newEnd > m_maxUsed) > m_maxUsed = newEnd; > > Index: JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp > =================================================================== > +#include <QDebug> > + > +namespace WTF { > + > +/** Efficiently allocates blocks of size blockSize with blockSize alignment. > + * Primarily designed for JSC RegisterFile's needs. > + * Not thread-safe. > + */ Just for my education, do other ports expect the RegisterFile object to shared across threads? > +ContigBlockAllocator::ContigBlockAllocator(TUint32 reservationSize, TUint32 blockSize ) > + : m_reserved(reservationSize), > + m_blockSize(blockSize) > +{ Somewhat subjective, but perhaps m_blockSize could be renamed to m_poolSize or similar? > + // 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_reserved = SYMBIAN_ROUNDUPTOMULTIPLE(m_reserved, m_pageSize); > + __ASSERT_ALWAYS(SYMBIAN_ROUNDUPTOMULTIPLE(m_blockSize, m_pageSize), User::Panic(_L("ContigBlockAllocator1"), KErrArgument)); > + > + // 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_reserved , EOwnerProcess); > + if (ret != KErrNone) > + User::Panic(_L("ContigBlockAllocator2"), 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()); So, we are really requesting our m_buffer to be blockSize aligned here. I don't think there is such a need for RegisterFile. At most, there may be a need to have sizeof(RegisterFile) alignment. Please correct me if I missed something. > + qDebug("CBA::CBA() handle=%d, reserved=%d, blocksize=%d", > + m_chunk.Handle(), m_reserved, m_blockSize); > + qDebug("CBA::CBA() base=%d, offset=%d", m_chunk.Base(), m_offset); > +} > + > +ContigBlockAllocator::~ContigBlockAllocator() > +{ > + // release everything! > + qDebug("CBA::~CBA() destroying everything!"); > + m_chunk.Decommit(0, m_chunk.MaxSize()); > + m_chunk.Close(); > +} > + > +void* ContigBlockAllocator::buffer() const > +{ > + void* buf = (void*)(TUint(m_chunk.Base()) + m_offset); > + qDebug("CBA::buffer() base=%d, offset=%d, buffer=%d ", m_chunk.Base(), m_offset, buf); > + return buf; > +} > + > +void ContigBlockAllocator::grow(void* newEnd) > +{ > + qDebug("CBA::grow() newEnd=%d", newEnd); > + > + // find the current end of the committed block > + void* curEnd = (void*)(m_chunk.Base() + m_offset + m_chunk.Size()); > + // find the end of the reserved block > + void* maxEnd = (void*)(m_chunk.Base() + m_chunk.MaxSize()); > + > + qDebug("CBA::grow() base=%d, offset=%d, committed=%d, curEnd=%d, maxEnd=%d", > + m_chunk.Base(), m_offset, m_chunk.Size(), curEnd, maxEnd); > + > + if (newEnd > maxEnd) { > + qDebug("CBA::grow() trying to commit more memory than reserved"); > + return; > + } > + > + if (newEnd > curEnd) { > + TInt nBytes = (TInt)(newEnd) - (TInt)(curEnd); > + nBytes = SYMBIAN_ROUNDUPTOMULTIPLE(nBytes, m_blockSize); > + TInt offset = (TInt)(curEnd) - (TInt)m_chunk.Base(); > + qDebug("CBA::grow() Committing %d bytes at offset %d", nBytes, offset); > + > + TInt ret = m_chunk.Commit(offset, nBytes); > + if (ret != KErrNone) > + qDebug("CBA::grow() failed! Commit: %d", ret); > + } > +} > + > +void ContigBlockAllocator::shrink(void* newEnd) > +{ > + qDebug("CBA::shrink() newEnd=%d", newEnd); > + > + // find the current end of the committed block > + void* curEnd = (void*)(m_chunk.Base() + m_offset + m_chunk.Size()); > + // find the end of the reserved block > + void* maxEnd = (void*)(m_chunk.Base() + m_chunk.MaxSize()); > + > + qDebug("CBA::shrink() base=%d, offset=%d, committed=%d, curEnd=%d, maxEnd=%d", > + m_chunk.Base(), m_offset, m_chunk.Size(), curEnd, maxEnd); > + > + if (newEnd < curEnd) { > + TInt nBytes = (TInt)(newEnd) - (TInt)(curEnd); > + if (nBytes < m_blockSize) > + return; > + > + TInt offset = SYMBIAN_ROUNDUPTOMULTIPLE((TUint)newEnd, m_blockSize) - (TInt)m_chunk.Base(); > + nBytes = (TInt)curEnd - offset - (TInt)m_chunk.Base(); > + > + if (nBytes > 0) { > + qDebug("CBA::shrink() Decommitting %d bytes at offset %d", nBytes, offset); > + > + TInt ret = m_chunk.Decommit(offset, nBytes); > + if (ret != KErrNone) > + qDebug("CBA::shrink() failed! Decommit: %d", ret); > + } > + } > +} > + > +} // end of namespace > + > +#endif // SYMBIAN > > Property changes on: JavaScriptCore/wtf/symbian/ContigBlockAllocator.cpp > ___________________________________________________________________ > Added: svn:executable > + * > > Index: JavaScriptCore/wtf/symbian/ContigBlockAllocator.h > =================================================================== > --- JavaScriptCore/wtf/symbian/ContigBlockAllocator.h (revision 0) > +++ JavaScriptCore/wtf/symbian/ContigBlockAllocator.h (revision 0) > @@ -0,0 +1,66 @@ > +/* > + * 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 ContigBlockAllocator_h > +#define ContigBlockAllocator_h > + > +#include "SymbianDefines.h" > + > +namespace WTF { > + > +/** > + * Allocates contiguous regions 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 ContigBlockAllocator { > + public: > + ContigBlockAllocator(TUint32 reservationSize, TUint32 blockSize); > + ~ContigBlockAllocator(); > + void* buffer() const; > + void grow(void* newEnd); > + void shrink(void* newEnd); > + > + private: > + RChunk m_chunk; // Symbian chunk that lets us reserve/commit/decommit > + > + // all following values are in numbers of bytes > + TInt m_pageSize; // cached value of system page size, typically 4K on Symbian > + TUint m_offset; // offset of first block from base > + TUint32 m_reserved; // total number of reserved bytes in virtual memory > + TUint32 m_blockSize; // size of one memory block, set by default to 64K in JSC/interpreter/RegisterFile.h > + > +}; > + > +} > + > +#endif // ContigBlockAllocator_h > + > + > > Property changes on: JavaScriptCore/wtf/symbian/ContigBlockAllocator.h > ___________________________________________________________________ > Added: svn:executable > + * > > Index: JavaScriptCore/wtf/symbian/SymbianDefines.h > =================================================================== > --- JavaScriptCore/wtf/symbian/SymbianDefines.h (revision 0) > +++ JavaScriptCore/wtf/symbian/SymbianDefines.h (revision 0) > @@ -0,0 +1,40 @@ > +/* > + * 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 SymbianDefines_h > +#define SymbianDefines_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) ) > + > +#endif // SymbianDefines_h > > Property changes on: JavaScriptCore/wtf/symbian/SymbianDefines.h > ___________________________________________________________________ > Added: svn:executable > + * > I will review everything after grow/shrink implementation later..
Created attachment 58485 [details] revised patch Here's a revised patch based on Siddharth's comments. One thing I haven't done is to move m_registerFileAllocator in RegisterFile from the heap to the stack. The reason is that its constructor needs to be initialized with the total number of bytes to reserve, which isn't calculated until later as the local variable bufferLength at the top of the RegisterFile constructor.
(In reply to comment #10) > Created an attachment (id=58485) [details] > revised patch Did you forget to mark it for review or is this a still work in progress patch?
The functionality of the patch is all there and appears to be working correctly, but there's still some debugging code left in. At the moment, I'd just like people to take a look and offer comments and suggestions for improvement, which I'll incorporate in a further go-around. Once everybody seems happy with the patch, I'll remove the debugging code from it and submit it for official review.
Comment on attachment 58485 [details] revised patch Looks good! Excited that this will save 4MB per JSC instance on a device :) --------------------------------- http://wkrietveld.appspot.com/34349/diff/1/5 File JavaScriptCore/interpreter/RegisterFile.h (right): http://wkrietveld.appspot.com/34349/diff/1/5#newcode161 JavaScriptCore/interpreter/RegisterFile.h:161: WTF::RegisterFileAllocator* m_registerFileAllocator; The code comments are incorrect, please fix. http://wkrietveld.appspot.com/34349/diff/1/5#newcode222 JavaScriptCore/interpreter/RegisterFile.h:222: m_buffer = (Register*)(m_registerFileAllocator->buffer()); Code comments need fixing, or may even be removed in this instance.
Created attachment 59110 [details] patch for review Here's a patch ready to be reviewed. I've also created a number of QtestLib-based test cases for the new RegisterFileAllocator class. I'll submit those test cases in a new bug.
I would like to see performance numbers (sunspider) from an actual Symbian device (most device will do) for this patch to make sure there is no significant performance degradation.
I wonder if it wouldn't be simpler to use the RChunk directly in RegisterFile? I admit there's a fair bit of #ifdeffery there and it's not pretty :) The new class on the other hand - RegisterFileAllocator - is not actually a platform independent interface as the name suggest. Instead it is Symbian specific.
Well, the (future-looking) intention is to make the RegisterFileAllocator class platform-independent. That's why the class is named RegisterFileAllocator, but the associated .h and .cpp files are called RegisterFileAllocatorSymbian. The idea is that each platform can define its own version of the RegisterFileAllocator class in its own RegisterFileAllocator<Platform> source files. That way, the only significant #ifdef stuff can take place at the very top of RegisterFile.h, where you choose which header file to include based on the platform. After that, you just use the RegisterFileAllocator class (which may be better thought of and even implemented as a pure virtual class, like a Java interface). You could then get rid of all the other #ifdef stuff like HAVE(MMAP) and HAVE(VIRTUALALLOC) and OS(WINCE) and so on and put all the platform-specific code into platform-specific source files: RegisterFileAllocatorMmap and so on. If anything, that will clean up the code in RegisterFile.h and make it easier to read. I've just taken the first step here since I didn't want to try to completely rewrite the RegisterFile class on my own just for the sake of this patch. I didn't think that that would go over so well in the community. :-)
(In reply to comment #14) > Created an attachment (id=59110) [details] > patch for review The changes in RegisterFile.h seem to be 99% indenting changes, which is completely unrelated to this issue.
(In reply to comment #17) > Well, the (future-looking) intention is to make the RegisterFileAllocator class platform-independent. That's why the class is named RegisterFileAllocator, but the associated .h and .cpp files are called RegisterFileAllocatorSymbian. > > The idea is that each platform can define its own version of the RegisterFileAllocator class in its own RegisterFileAllocator<Platform> source files. That way, the only significant #ifdef stuff can take place at the very top of RegisterFile.h, where you choose which header file to include based on the platform. After that, you just use the RegisterFileAllocator class (which may be better thought of and even implemented as a pure virtual class, like a Java interface). You could then get rid of all the other #ifdef stuff like HAVE(MMAP) and HAVE(VIRTUALALLOC) and OS(WINCE) and so on and put all the platform-specific code into platform-specific source files: RegisterFileAllocatorMmap and so on. > > If anything, that will clean up the code in RegisterFile.h and make it easier to read. > > I've just taken the first step here since I didn't want to try to completely rewrite the RegisterFile class on my own just for the sake of this patch. I didn't think that that would go over so well in the community. :-) The idea is good, but so long as the RegisterFileAllocator interface is not platform-neutral, the implementation is confusing. If you want to make this abstraction, why not try it with the existing platforms to make sure it'll actually work out OK. My recommendation would be to either 1) do what Simon suggested (ifdeffery directly in RegisterFile, no new classes); or 2) implement RegisterFileAllocator for the existing platforms (showing that your above idea works, i.e. getting rid of the ifdefs), and _then_ add the Symbian implementation of RegisterFileAllocator.
Comment on attachment 59110 [details] patch for review I agree with Kent's comments.
Does r64782 (https://bugs.webkit.org/show_bug.cgi?id=43185) fix this issue?
No more action required on this bug.