Bug 43515 - Fix small design issues with PageAllocation, split out PageReservation.
Summary: Fix small design issues with PageAllocation, split out PageReservation.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Blocks: 51108
  Show dependency treegraph
Reported: 2010-08-04 15:28 PDT by Gavin Barraclough
Modified: 2011-01-27 10:16 PST (History)
3 users (show)

See Also:

The patch (60.64 KB, patch)
2010-08-04 15:41 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-08-04 15:28:32 PDT
The PageAllocation class has a number of issues.

* Changes in bug #43269 accidentally switched SYMBIAN over to use malloc/free to allocate blocks of memory for the GC heap, instead of allocating RChunks.  Revert this change in behaviour.
* In order for PageAllocation to work correctly on WinCE we should be decommitting memory before deallocating.  In order to simplify understanding the expected state at deallocate, split behaviour out into PageAllocation and PageReservation classes.  Require that all memory be decommitted before calling deallocate on a PageReservation, add asserts to enforce this.
* add many missing asserts.
* inline more functions.
* remove ability to create sub-PageAllocations from an existing PageAllocations object – this presented an interface that would allow sub regions to be deallocated, which would not have provided expected behaviour.
* remove writable/executable arguments to commit, this value can be cached at the point the memory is reserved.
* remove writable/executable arguments to allocateAligned, protection other than RW is not supported.
* add missing checks for overflow & failed allocation to mmap path through allocateAligned.
Comment 1 Gavin Barraclough 2010-08-04 15:41:12 PDT
Created attachment 63504 [details]
The patch
Comment 2 WebKit Review Bot 2010-08-04 15:43:26 PDT
Attachment 63504 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/PageAllocation.h:49:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/PageAllocation.h:319:  system_info is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/PageAllocation.h:348:  page_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: 4 in 16 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gavin Barraclough 2010-08-04 17:22:25 PDT
Fixed in r64695.
Comment 4 Andrew Wilson 2010-08-04 17:44:57 PDT
Looks like this breaks the windows build:

1: c:\b\slave\webkit-rel-webkit-org\build\src\third_party\webkit\javascriptcore\wtf\PageReservation.h(220) : error C2597: illegal reference to non-static member 'WTF::PageReservation::m_protection'

2: c:\b\slave\webkit-rel-webkit-org\build\src\third_party\webkit\javascriptcore\wtf\PageReservation.h(221) : error C2597: illegal reference to non-static member 'WTF::PageReservation::m_protection'

I'm going to roll this out.
Comment 5 Adam Barth 2010-08-10 23:00:46 PDT
Comment on attachment 63504 [details]
The patch

Clearing sam's r+.  Looks like we'll need a new patch here.
Comment 6 Siddharth Mathur 2010-12-15 08:22:46 PST
Gavin, Could you please let us know if you intend to revisit this patch (the Symbian change specifically), or should we do it as part of Bug 51108? Either way is fine. Thanks!
Comment 7 Gavin Barraclough 2010-12-17 14:09:21 PST
Hi Siddharth,

I'm afraid I'm not going to have a chance to look at this again in the near future, I think Geoff has made some major changes to PageAllocation lately, so I doubt this patch can be merged up easily, I expect any fix will need a fresh implementation on ToT.

Comment 8 Siddharth Mathur 2011-01-27 10:16:52 PST
Resolving based on comment #7