486066 - Kill off the pres shell's stack arena

Status: NEW

(Core :: Layout, enhancement)

Description

[Zack Weinberg (:zwol)]

I'm gearing up for frame GC and I noticed that the pres shell actually carries *two* arenas, one of which is only used in two places (sprocket layout and table colspan handling) -- and its stack semantics made me initially think they would  trip over each other.  (They don't, but it's pretty subtle why not.)

This patch replaces each of those uses with a PLArena hosted directly in the code that was using it.  (I'm not a huge fan of PLArena's API but it seems to be what we have for this sort of thing.)

Comment 1

[Zack Weinberg (:zwol)]

Attachment: proposed patch

man, I just can't attach things tonight.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This patch seems to pretty much defeat the point of using arenas at all.  I don't think any one of these callers does any significant number of allocations; this patch means we're not only no longer fusing the allocations between the two different callers, but we stop fusing the allocations between the same caller at different points on the stack, which I think makes it not worth bothering with an arena at all.

That said, I somewhat liked the stack arena, and I'm not sure why we should get rid of it.  I think using it *more* would help us to avoid stack-overflow situations, since we do put an awful lot on the C++ call stack during reflow.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Well, the spanning cell sorter stuff isn't recursive, so there might be value in a standalone arena there, but I'm quite skeptical that a per-frame arena in nsBoxLayoutState is useful.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 370144 [details] [diff] [review]
proposed patch

This is really David's area of expertise.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 370144 [details] [diff] [review]
proposed patch

So I'm going to mark this minus based on what I said in comment 2; feel free to respond.  (I was hoping you'd jump in and respond sooner, but no point leaving this in my review queue when I'm reasonably confident it's not precisely what we want, even though I'm not sure whether on not we want to change something here)

Comment 6

[Zack Weinberg (:zwol)]

Yeah, sorry about that, I discovered I had a couple three higher priority bugs on my queue.

The only real advantage I see to having arena allocation in either sprocket layout or the table colspan sorter (given that jemalloc is supposed to be really good at tiny allocations anyway) is that it makes it easy to deallocate them all at once when we're done.  I'd be happy to switch to some other allocation scheme that provided that.  (Can nsTArray be used as a stack efficiently?)

The thing I don't like about the pres-shell stack arena is that it is used for two totally unrelated allocations (perhaps more in the future) and there's no structural reason why they can't step on each other.  I had to read the sprocket layout code very carefully to be confident that one of its little size objects could never be allocated in the middle of a table colspan sort operation (and thus be deallocated before it ought to.

I'm unenthusiastic about moving stuff off the C stack.  Stacks are big nowadays.

Comment 7

[Boris Zbarsky [:bzbarsky]]

All consumers of the stack arena should be ensuring stack behavior for their allocations and deallocations (that is, LIFO, and scoped to a particular function).  Is sprocket layout not doing that?

> Stacks are big nowadays.

Sort of.  We limit the depth of tree that we'll parse to a few hundred DOM nodes, because that's what we can handle before blowing out the stack during reflow or frame construction....  and that's on modern desktop OSes.  Not sure what the situation looks like on mobile stuff.

Comment 8

[Zack Weinberg (:zwol)]

(In reply to comment #7)
> All consumers of the stack arena should be ensuring stack behavior for their
> allocations and deallocations (that is, LIFO, and scoped to a particular
> function).  Is sprocket layout not doing that?

It is, it's just not obvious that it is, because there's this tangle of four classes involved (nsSprocketLayout itself, nsBoxLayoutState, nsBoxSize, and nsComputedBoxSize), the latter two of which are *unconditionally* allocated from the stack arena, so you have to read through all of the code and convince yourself that there isn't any way to get the situation I described in the previous comment.

Comment 9

[Zack Weinberg (:zwol)]

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.