Bug 43269

Summary: Change the JavaScript heap to use the new PageAllocation class
Product: WebKit Reporter: Nathan Lawrence <nlawrence>
Component: JavaScriptCoreAssignee: Nathan Lawrence <nlawrence>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, darin, gns, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 43207    
Attachments:
Description Flags
Patch
none
Patch
barraclough: review-
Patch
none
Patch
none
Patch (up to date)
barraclough: review-
Patch
barraclough: review+
Patch (up to date) none

Description Nathan Lawrence 2010-07-30 13:58:07 PDT
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.
Comment 1 Nathan Lawrence 2010-08-02 10:17:52 PDT
Created attachment 63229 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-02 10:19:56 PDT
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.
Comment 3 Early Warning System Bot 2010-08-02 10:32:51 PDT
Attachment 63229 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3560833
Comment 4 Darin Adler 2010-08-02 10:46:02 PDT
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
Comment 5 Nathan Lawrence 2010-08-02 10:50:40 PDT
Created attachment 63235 [details]
Patch

forgot to add some files to previous patch.
Comment 6 WebKit Review Bot 2010-08-02 10:54:30 PDT
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.
Comment 7 WebKit Review Bot 2010-08-02 18:45:36 PDT
Attachment 63235 [details] did not build on win:
Build output: http://queues.webkit.org/results/3558877
Comment 8 Gavin Barraclough 2010-08-03 00:58:12 PDT
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.
Comment 9 Nathan Lawrence 2010-08-03 12:15:04 PDT
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.
Comment 10 WebKit Review Bot 2010-08-03 12:19:55 PDT
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.
Comment 11 Early Warning System Bot 2010-08-03 12:32:22 PDT
Attachment 63365 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3607674
Comment 12 WebKit Review Bot 2010-08-03 14:01:41 PDT
Attachment 63365 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3566871
Comment 13 Nathan Lawrence 2010-08-03 14:41:02 PDT
Created attachment 63379 [details]
Patch

(Hopefully) fixed build issues + moved BlockAllocator to runtime per Gavin's suggestion.
Comment 14 Darin Adler 2010-08-03 14:55:04 PDT
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.
Comment 15 Darin Adler 2010-08-03 14:55:19 PDT
I mean rather than BLOCK_SIZE.
Comment 16 Nathan Lawrence 2010-08-03 14:57:47 PDT
Created attachment 63380 [details]
Patch (up to date)
Comment 17 Nathan Lawrence 2010-08-03 15:01:15 PDT
(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.
Comment 18 WebKit Review Bot 2010-08-03 15:03:43 PDT
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.
Comment 19 Gavin Barraclough 2010-08-03 15:26:20 PDT
Comment on attachment 63380 [details]
Patch (up to date)

As discussed, please make classname & filename, match, and remove reserveAligned.  This is looking awesome.
Comment 20 Nathan Lawrence 2010-08-03 15:44:50 PDT
Created attachment 63387 [details]
Patch
Comment 21 WebKit Review Bot 2010-08-03 15:47:20 PDT
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.
Comment 22 Gavin Barraclough 2010-08-03 16:55:27 PDT
Comment on attachment 63387 [details]
Patch

This is now pure awesome.
Comment 23 Nathan Lawrence 2010-08-03 18:22:37 PDT
Created attachment 63399 [details]
Patch (up to date)
Comment 24 WebKit Review Bot 2010-08-03 18:28:55 PDT
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.
Comment 25 Gavin Barraclough 2010-08-03 20:27:52 PDT
Fixed in r64624.
Comment 26 Oliver Hunt 2010-08-05 13:54:12 PDT
Comment on attachment 63399 [details]
Patch (up to date)

clearing review flag