Closed Bug 639743 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(6 files)

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().
Attachment #517649 - Flags: review?(dvander)
Attached patch patch 2Splinter Review
Attachment #517650 - Flags: review?(dvander)
Attached patch patch 3Splinter Review
Attachment #517651 - Flags: review?(dvander)
Attached patch patch 4Splinter Review
Attachment #517652 - Flags: review?(dvander)
Attached patch patch 5Splinter Review
Attachment #517653 - Flags: review?(dvander)
Attached patch patch 6Splinter Review
Attachment #517654 - Flags: review?(dvander)
I <3 njn-cleanup. That is all.

/be
Attachment #517649 - Flags: review?(dvander) → review+
Attachment #517650 - Flags: review?(dvander) → review+
Comment on attachment 517651 [details] [diff] [review]
patch 3


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

Should this be static?
Attachment #517651 - Flags: review?(dvander) → review+
Attachment #517652 - Flags: review?(dvander) → review+
Attachment #517653 - Flags: review?(dvander) → review+
Attachment #517654 - Flags: review?(dvander) → review+
Depends on: 657164
You need to log in before you can comment on or make changes to this bug.