1351904 - Switch layout over to ArenaAllocator

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Eric Rahm [:erahm]]

Attachment: Switch layout over to ArenaAllocator

Comment 2

[Eric Rahm [:erahm]]

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 3

[Eric Rahm [:erahm]]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 5

[Eric Rahm [:erahm]]

(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).

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(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 :)

Comment 7

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/102d5db4cb8c74513fd11b53e8f2d743aa40578a
Bug 1351904 - Switch layout over to ArenaAllocator. r=xidorn

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/102d5db4cb8c