Bug 43269 – Change the JavaScript heap to use the new PageAllocation class

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Nathan Lawrence]

The javascript heap relies on being able to allocate aligned CollectorBlocks.  Currently that functionality is split between the symbian implementation in WTF and platform specific implementations inside Heap::allocateBlock and Heap::freeBlockPtr.  The suggested solution to this is to add a PageAllocation::allocateAligned method for platforms which support it, and then add a wrapper layer with the same API as the symbian AlignedBlockAllocator with a thin implementation for platforms which support aligned allocations and a fallback implementation for platforms which don't.

Such an api could also be used for https://bugs.webkit.org/show_bug.cgi?id=43207.

[Nathan Lawrence]

Created attachment 63229 [details]
Patch

[WebKit Review Bot]

Attachment 63229 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/PageAllocation.h:68:  "sys/mman.h" already included at JavaScriptCore/wtf/PageAllocation.h:43  [build/include] [4]
JavaScriptCore/wtf/PageAllocation.h:70:  "unistd.h" already included at JavaScriptCore/wtf/PageAllocation.h:44  [build/include] [4]
JavaScriptCore/runtime/Collector.h:76:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:78:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 63229 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3560833

[Darin Adler]

Qt failures are:

../../../JavaScriptCore/runtime/Collector.h:27:24: error: wtf/Bitmap.h: No such file or directory
../../../JavaScriptCore/runtime/Collector.h:36:37: error: wtf/TypedPageAllocation.h: No such file or directory
../../../JavaScriptCore/runtime/Collector.h:42:32: error: wtf/BlockAllocator.h: No such file or directory

[Nathan Lawrence]

Created attachment 63235 [details]
Patch

forgot to add some files to previous patch.

[WebKit Review Bot]

Attachment 63235 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:76:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:78:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 63235 [details] did not build on win:
Build output: http://queues.webkit.org/results/3558877

[Gavin Barraclough]

Comment on attachment 63235 [details]
Patch

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.

[Nathan Lawrence]

Created attachment 63365 [details]
Patch

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

[WebKit Review Bot]

Attachment 63365 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:59:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:61:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 63365 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3607674

[WebKit Review Bot]

Attachment 63365 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3566871

[Nathan Lawrence]

Created attachment 63379 [details]
Patch

(Hopefully) fixed build issues + moved BlockAllocator to runtime per Gavin's suggestion.

[Darin Adler]

Comment on attachment 63379 [details]
Patch

If we are moving the block size constant to a header, I suggest naming it blockSize rather than BlockSize. Or collectorBlockSize or even CollectorBlockSize.

[Darin Adler]

I mean rather than BLOCK_SIZE.

[Nathan Lawrence]

Created attachment 63380 [details]
Patch (up to date)

[Nathan Lawrence]

(In reply to comment #14)
> (From update of attachment 63379 [details])
> If we are moving the block size constant to a header, I suggest naming it blockSize rather than BlockSize. Or collectorBlockSize or even CollectorBlockSize.

BLOCK_SIZE is just being moved around in the header.  I intend to submit another patch that puts all of the constants into the HeapConstants struct (such that the unused ones are private) after this patch goes through.

[WebKit Review Bot]

Attachment 63380 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:60:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:62:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 63380 [details]
Patch (up to date)

As discussed, please make classname & filename, match, and remove reserveAligned.  This is looking awesome.

[Nathan Lawrence]

Created attachment 63387 [details]
Patch

[WebKit Review Bot]

Attachment 63387 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:60:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:62:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 63387 [details]
Patch

This is now pure awesome.

[Nathan Lawrence]

Created attachment 63399 [details]
Patch (up to date)

[WebKit Review Bot]

Attachment 63399 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/Collector.h:60:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/runtime/Collector.h:62:  BLOCK_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Fixed in r64624.

[Oliver Hunt]

Comment on attachment 63399 [details]
Patch (up to date)

clearing review flag