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)

enhancement
Not set
normal

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
Component: Layout: Text → CSS Parsing and Computation
See Also: → 1355600
(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 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+
Keywords: checkin-needed
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+
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
Thanks, RyanVM (and glob for the issue-count bug fixup).
https://hg.mozilla.org/mozilla-central/rev/756aa07a1852
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.