Represent TextOverflow as Maybe<> instead of UniquePtr

RESOLVED WONTFIX

Status

()

Core
Layout: Text
RESOLVED WONTFIX
8 months ago
8 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
Right now we allocate TextOverflow instances on the heap (inside of  TextOverflow::WillProcessLines).

But there's no reason for it to live on the heap, AFAICT -- its lifetime is scoped using an RAII guard object (a UniquePtr local variable), and it only lives for the duration of a nsBlockFrame::BuildDisplayList call. (Nobody else takes ownership.)

I suspect the only reason it's heap-allocated is so we can have an optional "null" state.  We can just as easily represent this using Maybe<>, with stack allocation.

So, I propose we should do away with the heap allocation here and just make WillProcessLines return Maybe<TextOverflow>, and make nsBlockFrame::BuildDisplayList (and its helpers) deal with that type as well (which can mostly be treated as pointer-ish).
(Assignee)

Comment 1

8 months ago
The patches here will be layered on top of bug 944200's patches.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 944200
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

8 months ago
(Sorry for the churn -- I added some "const" annotations at the last minute before posting the first version of the patches, with the intent of expressing "do not reset this Maybe<> instance once it's been emplaced".  But Maybe transfers that const-ness to the thing that it wraps, so this change didn't compile, because we call non-const methods on the Maybe<TextOverflow>.  

So, I've now reverted the "const" annotations that the initial version of this patch had added.)

Comment 6

8 months ago
mozreview-review
Comment on attachment 8857056 [details]
Bug 1355531: Represent temporary TextOverflow instances using Maybe instead of UniquePtr.

https://reviewboard.mozilla.org/r/128956/#review133096

I'm not convinced this is an improvement.  In a Linux64 Opt build:

(gdb) p sizeof(mozilla::css::TextOverflow)
$1 = 128

So Maybe<TextOverflow> is at least 129 bytes extra on the stack for all
calls to nsBlockFrame::BuildDisplayList that are currently on the stack,
even if 'text-overflow' isn't used for the block.  IIRC, BuildDisplayList
calls can be quite deeply nested so there's potentially many kilobytes of
stack space added for this variable.  'text-overflow' is quite rare so
most of that space will be unused.  I think stack space is a more
precious resource than heap space so this seems like a waste to me.

I guess we could allocate TextOverflow from the shell arena to avoid
malloc churn, but I doubt that's justified given that 'text-overflow'
is quite rare.  So, I think the current code is optimal.
Attachment #8857056 - Flags: review?(mats) → review-
(Assignee)

Comment 7

8 months ago
(In reply to Mats Palmgren (:mats) from comment #6)
> 'text-overflow' is quite rare so
> most of that space will be unused.

Thanks - this is a very good point. Agreed, no need to change things here then.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.