|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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.
(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.
(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.