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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Nathan Lawrence <nlawrence at apple.com>
(In reply to comment #8)
> (From update of attachment 63235 [details])
> 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)));].

See https://bugs.webkit.org/show_bug.cgi?id=29379 for aligned allocation on
Windows CE.  I'm removing that code path.  So what should the correct behavior
there be?  There are two implementations of aligned allocate that would work. 
There is the default allocateAligned in PageAllocator, and then there is the
AlignedBlockAllocator implementation that reserves a large chunk of virtual
memory.

> 
> 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'.

Having a variable 'one' allows me to have the comment in a logical place and
ensures that it will be of the correct type.

> 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?

I have the destroy method there because it is called from Heap::destroy.

> > 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)));

Good catch.

> > 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.

Fixed.

> 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.

I should be able to take full advantage of any of the generic classes in the
same way when I submit the new patch to
https://bugs.webkit.org/show_bug.cgi?id=43207.	That said, the code is simple
enough where it shouldn't be too much of a problem duplicating it, and I have
removed TypedPageAllocation.

> 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.

reservationSize has been removed.

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

I really do appreciate all of the feedback.
 
> cheers,
> G.