Closed Bug 1355531 Opened 7 years ago Closed 7 years ago

Represent TextOverflow as Maybe<> instead of UniquePtr

Categories

(Core :: Layout: Text and Fonts, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox55 --- affected

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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).
The patches here will be layered on top of bug 944200's patches.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 944200
(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 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-
(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
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: