WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
43269
Change the JavaScript heap to use the new PageAllocation class
https://bugs.webkit.org/show_bug.cgi?id=43269
Summary
Change the JavaScript heap to use the new PageAllocation class
Nathan Lawrence
Reported
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
.
Attachments
Patch
(21.53 KB, patch)
2010-08-02 10:17 PDT
,
Nathan Lawrence
no flags
Details
Formatted Diff
Diff
Patch
(34.69 KB, patch)
2010-08-02 10:50 PDT
,
Nathan Lawrence
barraclough
: review-
Details
Formatted Diff
Diff
Patch
(46.54 KB, patch)
2010-08-03 12:15 PDT
,
Nathan Lawrence
no flags
Details
Formatted Diff
Diff
Patch
(46.12 KB, patch)
2010-08-03 14:41 PDT
,
Nathan Lawrence
no flags
Details
Formatted Diff
Diff
Patch (up to date)
(46.23 KB, patch)
2010-08-03 14:57 PDT
,
Nathan Lawrence
barraclough
: review-
Details
Formatted Diff
Diff
Patch
(47.73 KB, patch)
2010-08-03 15:44 PDT
,
Nathan Lawrence
barraclough
: review+
Details
Formatted Diff
Diff
Patch (up to date)
(47.45 KB, patch)
2010-08-03 18:22 PDT
,
Nathan Lawrence
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Nathan Lawrence
Comment 1
2010-08-02 10:17:52 PDT
Created
attachment 63229
[details]
Patch
WebKit Review Bot
Comment 2
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.
Early Warning System Bot
Comment 3
2010-08-02 10:32:51 PDT
Attachment 63229
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/3560833
Darin Adler
Comment 4
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
Nathan Lawrence
Comment 5
2010-08-02 10:50:40 PDT
Created
attachment 63235
[details]
Patch forgot to add some files to previous patch.
WebKit Review Bot
Comment 6
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.
WebKit Review Bot
Comment 7
2010-08-02 18:45:36 PDT
Attachment 63235
[details]
did not build on win: Build output:
http://queues.webkit.org/results/3558877
Gavin Barraclough
Comment 8
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.
Nathan Lawrence
Comment 9
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.
WebKit Review Bot
Comment 10
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.
Early Warning System Bot
Comment 11
2010-08-03 12:32:22 PDT
Attachment 63365
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/3607674
WebKit Review Bot
Comment 12
2010-08-03 14:01:41 PDT
Attachment 63365
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/3566871
Nathan Lawrence
Comment 13
2010-08-03 14:41:02 PDT
Created
attachment 63379
[details]
Patch (Hopefully) fixed build issues + moved BlockAllocator to runtime per Gavin's suggestion.
Darin Adler
Comment 14
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.
Darin Adler
Comment 15
2010-08-03 14:55:19 PDT
I mean rather than BLOCK_SIZE.
Nathan Lawrence
Comment 16
2010-08-03 14:57:47 PDT
Created
attachment 63380
[details]
Patch (up to date)
Nathan Lawrence
Comment 17
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.
WebKit Review Bot
Comment 18
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.
Gavin Barraclough
Comment 19
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.
Nathan Lawrence
Comment 20
2010-08-03 15:44:50 PDT
Created
attachment 63387
[details]
Patch
WebKit Review Bot
Comment 21
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.
Gavin Barraclough
Comment 22
2010-08-03 16:55:27 PDT
Comment on
attachment 63387
[details]
Patch This is now pure awesome.
Nathan Lawrence
Comment 23
2010-08-03 18:22:37 PDT
Created
attachment 63399
[details]
Patch (up to date)
WebKit Review Bot
Comment 24
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.
Gavin Barraclough
Comment 25
2010-08-03 20:27:52 PDT
Fixed in
r64624
.
Oliver Hunt
Comment 26
2010-08-05 13:54:12 PDT
Comment on
attachment 63399
[details]
Patch (up to date) clearing review flag
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug