Closed Bug 1351904 Opened 3 years ago Closed 3 years ago

Switch layout over to ArenaAllocator

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file)

This switches over layout's usage of PLArena to ArenaAllocator. This allows
us to build more files in unified sources and gets rid of various CONST masks.

MozReview-Commit-ID: Aaf3Dl2kaoz
Attachment #8852721 - Flags: review?(dbaron)
Comment on attachment 8852721 [details] [diff] [review]
Switch layout over to ArenaAllocator

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

::: layout/generic/nsLineLayout.cpp
@@ +380,5 @@
>  {
>    nsLineLayout* outerLineLayout = GetOutermostLineLayout();
>    PerSpanData* psd = outerLineLayout->mSpanFreeList;
>    if (!psd) {
> +    void *mem = outerLineLayout->mArena.Allocate(sizeof(PerSpanData));

Note: |ArenaAllocator::Allocate| is infallible by default.

::: view/moz.build
@@ -11,5 @@
>      'nsView.h',
>      'nsViewManager.h',
>  ]
>  
> -# nsViewManager.cpp cannot be built in unified mode because it uses PL_ARENA_CONST_ALIGN_MASK.

...but didn't actually do anything with it :)
Comment on attachment 8852721 [details] [diff] [review]
Switch layout over to ArenaAllocator

Xidorn, let me know if you have any questions. The blocking bug has the actual ArenaAllocator implementation.
Attachment #8852721 - Flags: review?(dbaron) → review?(xidorn+moz)
Comment on attachment 8852721 [details] [diff] [review]
Switch layout over to ArenaAllocator

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

Looks reasonable to me.

::: layout/base/nsPresArena.h
@@ +155,5 @@
>      enum { ALLOW_MEMMOVE = false };
>    };
>  
>    nsTHashtable<FreeList> mFreeLists;
> +  mozilla::ArenaAllocator<8192,8> mPool;

Please add a whitespace after the comma.

::: layout/generic/nsLineLayout.cpp
@@ +76,5 @@
>      mDirtyNextLine(false),
>      mLineAtStart(false),
>      mHasRuby(false),
> +    mSuppressLineWrap(nsSVGUtils::IsInSVGTextSubtree(aOuterReflowInput->mFrame)),
> +    mArena()

Given ArenaAllocator has default constructor, I don't think you need this line.

::: layout/painting/nsDisplayList.cpp
@@ +875,5 @@
>      nsDisplayListBuilderMode aMode, bool aBuildCaret)
>      : mReferenceFrame(aReferenceFrame),
>        mIgnoreScrollFrame(nullptr),
>        mLayerEventRegions(nullptr),
> +      mPool(),

Ditto.

::: layout/style/nsCSSRuleProcessor.cpp
@@ +3508,5 @@
>        mCounterStyleRules(aCounterStyleRules),
>        mDocumentRules(aDocumentRules),
>        mCacheKey(aKey),
>        mDocumentCacheKey(aDocumentKey),
> +      mArena(),

Ditto.

::: view/moz.build
@@ -11,5 @@
>      'nsView.h',
>      'nsViewManager.h',
>  ]
>  
> -# nsViewManager.cpp cannot be built in unified mode because it uses PL_ARENA_CONST_ALIGN_MASK.

So we cannot move it to UNIFIED_SOURCES even if it doesn't use PLArena?
Attachment #8852721 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Comment on attachment 8852721 [details] [diff] [review]
> Switch layout over to ArenaAllocator
> 
> Review of attachment 8852721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me.

Thanks for the quick review, I'll make the changes you suggested.

> So we cannot move it to UNIFIED_SOURCES even if it doesn't use PLArena?

I moved it, note change from SOURCES -> UNIFIED_SOURCES (those are the only files).
(In reply to Eric Rahm [:erahm] from comment #5)
> > So we cannot move it to UNIFIED_SOURCES even if it doesn't use PLArena?
> 
> I moved it, note change from SOURCES -> UNIFIED_SOURCES (those are the only
> files).

Oh, right. There are only two files there so just change SOURCES to UNIFIED_SOURCES :)
https://hg.mozilla.org/mozilla-central/rev/102d5db4cb8c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.