Closed
Bug 1383925
Opened 7 years ago
Closed 7 years ago
nsRuleNode::SetFont wastes time creating a temporary nsFont (and copying into it) for no good reason
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: dholbert, Assigned: kevin.hsieh)
References
Details
Attachments
(1 file)
The function nsRuleNode::SetFont creates a local nsFont variable, and then copies another nsFont into it: CODE QUOTE: // Fall back to defaultVariableFont. nsFont systemFont = *defaultVariableFont; const nsCSSValue* systemFontValue = aRuleData->ValueForSystemFont(); if (eCSSUnit_Enumerated == systemFontValue->GetUnit()) { LookAndFeel::FontID fontID = (LookAndFeel::FontID)systemFontValue->GetIntValue(); ComputeSystemFont(&systemFont, fontID, aPresContext, defaultVariableFont); } Link: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#3692-3699 (Here, defaultVariableFont is a pointer to a more persistent nsFont object -- and systemFont matches it by default, but in the unlikely scenario that "systemFontValue" is set, then we populate it with some different data.) This is silly -- most of the time, the copy is unchanged so we might as well just be directly operating on |defaultVariableFont|. (And the copy/destruction can be expensive in some cases, particularly if defaultVariableFont has non-empty nsTArray member variables). I think we can do something like the following instead, using "Maybe"[1] to let us have a local variable which only gets instantiated/copied if we need it: nsFont* systemFont = defaultVariableFont; Maybe<nsFont> lazySystemFont; [...] if (eCSSUnit_Enumerated == systemFontValue->GetUnit()) { lazySystemFont.emplace(*defaultVariableFont); systemFont = &lazySystemFont; [...old code resumes here...] } And then all mentions of "systemFont.[something]" would need to change to use -> instead of '.' Kevin, perhaps you could take this? [1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Maybe.h
Reporter | ||
Updated•7 years ago
|
Component: Layout: Text → CSS Parsing and Computation
Reporter | ||
Comment 1•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > if (eCSSUnit_Enumerated == systemFontValue->GetUnit()) { > lazySystemFont.emplace(*defaultVariableFont); > systemFont = &lazySystemFont; (Er, sorry, my "&" usage here was wrong. This ^^ probably wants to be "systemFont = lazySystemFont.ptr();" I was misremembering a bit -- Maybe<> has some convenience operators to make it behave kind of like a pointer, semantically, but I don't think it has operator&. Also, operator& wouldn't even make sense in this case anyway, since it'd be semantically getting the address *of* the pointer-like thing, which is an extra level of indirection.)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8889948 [details] Bug 1383925 - Remove unnecessary copy construction of default font in nsRuleNode.cpp. https://reviewboard.mozilla.org/r/161010/#review166302 r=me, with 1 nit addressed: ::: layout/style/nsRuleNode.cpp:26 (Diff revision 1) > #include "mozilla/Likely.h" > #include "mozilla/LookAndFeel.h" > #include "mozilla/Maybe.h" > #include "mozilla/OperatorNewExtensions.h" > #include "mozilla/Unused.h" > +#include "mozilla/Maybe.h" This new #include is unneeded -- we've already got this header a few lines up (at the correct sorted position). So, please remove it. (Thanks for thinking to add it, though, in case it was needed!)
Attachment #8889948 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20cecda296ca80357ca26aac1aa2bfc235deb89d
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8889948 [details] Bug 1383925 - Remove unnecessary copy construction of default font in nsRuleNode.cpp. https://reviewboard.mozilla.org/r/161010/#review166378
Attachment #8889948 -
Flags: review+
Comment 7•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/756aa07a1852 Remove unnecessary copy construction of default font in nsRuleNode.cpp. r=dholbert
Reporter | ||
Comment 9•7 years ago
|
||
Thanks, RyanVM (and glob for the issue-count bug fixup).
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/756aa07a1852
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•