Closed Bug 1272449 Opened 4 years ago Closed 4 years ago

Finish the allocator refactoring

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

This moves the rest of the allocation code into Allocator.cpp and makes the following trivial changes:

* return TenuredCell from refillFreeList
* use refillFreeListInGC to implement tenuring allocations

The rest is just code motion.

With this, I believe that all of the core allocation logic is pretty well isolated in Allocator.cpp.
Attachment #8751851 - Flags: review?(jcoppeard)
Blocks: 988356
Comment on attachment 8751851 [details] [diff] [review]
00_finish_moving_to_Allocator-v0.diff

Review of attachment 8751851 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: js/src/gc/Allocator.cpp
@@ +358,5 @@
> +     */
> +
> +    zone->arenas.checkEmptyFreeList(thingKind);
> +    mozilla::DebugOnly<JSRuntime*> rt = zone->runtimeFromMainThread();
> +    MOZ_ASSERT(rt->isHeapMajorCollecting() || rt->isHeapMinorCollecting());

You can just assert rt->isHeapCollecting() here.

@@ +434,5 @@
>      }
>  }
> +
> +
> +// ///////////  Chunk -> Arena Allocator  //////////////////////////////////////

Total nit, but we should probably standardise this kind of 'banner comment'.  I know I use something different for this.

@@ +531,5 @@
> +uint32_t
> +Chunk::findDecommittedArenaOffset()
> +{
> +    /* Note: lastFreeArenaOffset can be past the end of the list. */
> +    for (unsigned i = info.lastDecommittedArenaOffset; i < ArenasPerChunk; i++)

Pre-existing, but these for loops need braces.
Attachment #8751851 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/mozilla-central/rev/00d30d448554
https://hg.mozilla.org/mozilla-central/rev/418fd092a81a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.