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)
Core
Layout: Text and Fonts
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).
Assignee | ||
Comment 1•7 years ago
|
||
The patches here will be layered on top of bug 944200's patches.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•