639743 - JM: clean up Executable{Pool,Allocator} some more

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Here is a stack of patches that smooth some rough edges off ExecutablePool
and ExecutableAllocator.

one:
- In ExecutableAllocator::create(), don't eagerly allocate the first pool,
  let it happen lazily in poolForSize() instead.  This makes
  ExecutableAllocator::create() infallible, and so it can be changed to a
  normal constructor.

two:
- Adds ExecutableAllocator::createPool(), which calls systemAlloc() instead
  of ExecutablePool().  This makes ExecutablePool() infallible, which allows
  Executable::create() to be dispensed with.

three:
- Moves roundUpAllocationSize() into ExecutableAllocator, possible thanks to
  the above changes.

four:
- Renames the misspelt intializePageSize() as determinePageSize(), and make
  it just return a value instead of doing an assignment.

five:
- Replaces JIT_ALLOCATOR_PAGE_SIZE with pageSize, which is already what it's
  defined as.  Replaces JIT_ALLOCATOR_LARGE_ALLOC_SIZE with the newly added
  largeAllocSize.  Make them both private.

six:
- Makes ExecutablePool()::addRef() private.
- Changes ~ExecutableAllocator() to call release() instead of destroy() on
  the small allocation pools -- it should have the same effect as these
  should be the last references.  A boolean arg to release() is used to
  assert this.
- Inlines destroy() now that there's only one caller of it.

After all these changes:
- Both classes have normal constructors (no create() methods required).
- The only public method in ExecutablePool is release().
- The only public methods in ExecutableAllocator are the constructor and
  destructor, alloc(), and makeWritable/makeExecutable/cacheFlush().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 1 (against TM 63048:72f078c33771)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch 2

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch 3

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch 4

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch 5

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch 6

Comment 7

[Brendan Eich [:brendan]]

I <3 njn-cleanup. That is all.

/be

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 517651 [details] [diff] [review]
patch 3


>+    size_t roundUpAllocationSize(size_t request, size_t granularity)
>+    {

Should this be static?

Comment 9

[Nicholas Nethercote [inactive]]

I changed roundUpAllocationSize() to be static:

http://hg.mozilla.org/tracemonkey/rev/5bfe25660c9e
http://hg.mozilla.org/tracemonkey/rev/504939f68ed2
http://hg.mozilla.org/tracemonkey/rev/03151dfd6663
http://hg.mozilla.org/tracemonkey/rev/5a256e0cd7b6
http://hg.mozilla.org/tracemonkey/rev/7c1b798a25fd
http://hg.mozilla.org/tracemonkey/rev/bb0dd1dcfada

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/5bfe25660c9e
http://hg.mozilla.org/mozilla-central/rev/504939f68ed2
http://hg.mozilla.org/mozilla-central/rev/03151dfd6663
http://hg.mozilla.org/mozilla-central/rev/5a256e0cd7b6
http://hg.mozilla.org/mozilla-central/rev/7c1b798a25fd
http://hg.mozilla.org/mozilla-central/rev/bb0dd1dcfada