[webkit-reviews] review denied: [Bug 43269] Change the JavaScript heap to use the new PageAllocation class : [Attachment 63235] Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Nathan Lawrence
<nlawrence at apple.com>'s request for review:
Bug 43269: Change the JavaScript heap to use the new PageAllocation class
https://bugs.webkit.org/show_bug.cgi?id=43269

Attachment 63235: Patch
https://bugs.webkit.org/attachment.cgi?id=63235&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
Hey Nathan,

There are a lot of review comments here, but only because I think this is
really worth getting right – this is an absolutely fantastic clean-up of the
collector, thank you.

Looking at the allocateAligned methods, one thing is bothering me, which is the
comment in the #if OS(WINCE) code path.  Reading up on the behaviour of
VirtualAlloc, I haven't yet found any evidence that this is safe. :-/  That
said presumably it is, or else I don't think WebKit could work on WinCE.  So,
assuming that this works (if we don't assume this we should delete the WinCE
implementation!), it's probably a good idea to define the interface of this
method to be something we can support on all platforms we're implementing it on
– which is to say that if on WinCE we can only provide allocations that are
aligned to size, then we should probably change the interface to
allocateAligned to remove the alignment mask parameter, and to instead require
that size is a power of two [ASSERT(!(size & (size - 1)));], and align to (size
- 1).  Checking in allocateAligned with a faulty implementation on one platform
is not a great option.	We should  also
[ASSERT(!(static_cast<intptr_t>(result_of_VirtualAlloc) & (size - 1)));].

In the !HAVE(ALIGNED_ALLOCATE) version of AlignedBlockAllocator, please switch
away from creating sub-PageAllocations from the PageAllocation, and instead
return your own type (say, called 'Allocation')to hold a base pointer (in the
HAVE(ALIGNED_ALLOCATE) version Allocation could typedef to PageAllocation).  I
think we are going to need to remove the ability to allocate sub-allocations,
since exposing an interface which would appear to allow you to deallocate
regions is a bad idea (since we won't get the right behaviour on Symbian).

In BitMap, instead of declaring the value 'one', I'd suggest you should
probably just use '1u'.

In AlignedBlockAllocator, I'd suggest the number of #ifdefs is currently making
it much less readable; I'd suggest one big #if HAVE(ALIGNED_ALLOCATE) wrapping
(two copies of) the whole class definition would be cleaner in this case. 
Class member names must begin with m_, and the filename must match the class
name.  In AlignedBlockAllocator I don't see a need for a destroy method, the
contents could just move into the destructor?

> 91	 ASSERT(!(blockSize & PageAllocation::pagesize()));
> 92	 ASSERT(!(reservationSize & blockSize));

I think these asserts are incorrect; they check blockSize !=
PageAllocation::pagesize() and reservationSize != blockSize.  I think they
should be:

> 91	 ASSERT(!(blockSize & (PageAllocation::pagesize() - 1)));
> 92	 ASSERT(!(reservationSize & (blockSize - 1)));

> 109	  reservation.decommit(reservation.base(), reservation.size());

On OS X I think this would be an error; I believe we need to match calls to
commit/decommit for the kernel to be able to track memory usage correctly; we
should not be called decommit on memory that has not been committed.  Since I'm
assuming this decommit is in the existing code I'm not going to ask you to fix
this, but a FIXME comment describing the issue would be good.

Also, I think your patch should be removing the
wtf/symbian/BlockAllocatorSymbian* classes?

Finally, the use of templates.	In these cases, I don't think it works. 
Templates are great where you can create a generic class that will be useful
elsewhere, and as such where you can reduce code duplication - but you have to
be careful not to over-engineer and end up with a solution that is more complex
than necessary.

Looking at the class TypedPageAllocation, it sounds like it has potential to be
useful, but looking at similar situations in other Allocators, it doesn't seem
like this abstraction is a fit (in ExecutableAllocator no management structures
are stored within the mapped memory, in the BumpAllocator they are, but the
object representing the block is stored at the end of the allocation, and the
PageAllocation handle is stored with the allocation rather than being used as
the handle to the object).  Given the number of lines of code required to add
this class, and the unwieldiness of the type
'TypedPageAllocation<CollectorBlock>', I'd suggest instead of adding this class
you could just add a method:

Collector::collectorBlock(size_t index)
{
	return static_cast<CollectorBlock*>(blocks[index].base());
}

And in almost all cases use of "blocks[n]->" could just turn to
"collectorBlock(n)->".	In lieu of an obvious widespread use for
TypedPageAllocation I'd suggest that a simpler solution like this is much more
elegant.

As for AlignedBlockAllocator, given that the code was not previously
parameterized on the reservationSize it doesn't seem necessary to do so now,
and it's not great to have the type (specifically the template parameters) of
AlignedBlockAllocator change between the HAVE(ALIGNED_ALLOCATE) & non- builds,
so at minimum I'd remove this template parameter.  That said, personally I'd go
further.  We don't have any use for the aligned allocator outside of the
Collector, and I'm not sure that we really envisage any other uses.  Since we
have no other uses, it is hard to say if this interface is appropriate for
other uses.  As such it's hard to see any benefit in having a generalized
class.	Again, preferring simplicity over unnecessary complexity, I'd suggest
it would be better to simply rename the class 'CollectorAllocator', move it
into the runtime, and hardcode the use of BLOCK_SIZE and
JSCOLLECTOR_VIRTUALMEM_RESERVATION directly.  I'm not going to require this
change, since the existing code has AlignedBlockAllocator generalized in wtf,
but I do think it would be an improvement, and I am going to ask you to at
least remove reservationSize as a template parameter so the type is the same in
all builds.

Again, I know there are a a lot of review comments here, but this is a really
great change Nathan.

cheers,
G.