862763 - nsLayoutUtils::FontSizeInflationEnabled shows up in profiles, but shouldn't

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsLayoutUtils::FontSizeInflationEnabled keeps showing up in profiles of reflow, but there's no reason for it to do so, since it's basically constant for a given pres context (though it could change when the screen size or viewport info changes).  This should instead be cached on the pres context or pres shell and invalidated when necessary, since computing it is nontrivial (given the number of times it's called).

(And, given that, the code should just live in pres context or pres shell rather than layout utils.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should fix this on the b2g18 branch given that this is a decent percentage of (15% in one profile jrmuizel did of the contacts app) reflow performance in B2G profiles, and it's pretty trivial to fix.


Scott, would you mind taking this?

Comment 2

[Scott Johnson (:jwir3)]

Sure, I'll take it.

Comment 3

[Andrew Overholt [:overholt]]

Scott/David, triage is interested in how risky this is before making a decision to take this on b2g18.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think it's reasonably low risk.  The main risk is failing to update the per-document enabled state when we ought to, e.g., when a <meta viewport> that should disable font inflation is added.  I think this is reasonably easy to test, though.

Comment 5

[Alex Keybl [:akeybl]]

We'll start this as a leo+ bug and then consider it as tef+ once resolved. We don't want this being worked on as a higher priority than other tef+ bugs (if Scott has any).

Comment 6

[Scott Johnson (:jwir3)]

Attachment: b862763

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #4)
> I think it's reasonably low risk.  The main risk is failing to update the
> per-document enabled state when we ought to, e.g., when a <meta viewport>
> that should disable font inflation is added.  I think this is reasonably
> easy to test, though.

The general case of this is already tested in some of the reftests in layout/reftests/font-inflation, so I didn't add any additional tests, but I'd be happy to if you think there's code area that hasn't been tested here, David.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 741034 [details] [diff] [review]
b862763

> nsDocument::InsertChildAt(nsIContent* aKid, uint32_t aIndex,
>                           bool aNotify)
> {
>   if (aKid->IsElement() && GetRootElement()) {
>     NS_ERROR("Inserting element child when we already have one");
>     return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>   }
> 
>-  return doInsertChildAt(aKid, aIndex, aNotify, mChildren);
>+  nsresult rv = doInsertChildAt(aKid, aIndex, aNotify, mChildren);
>+  if (NS_SUCCEEDED(rv) &&
>+      (aKid->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE)) {
>+    nsCOMPtr<nsIPresShell> shell = GetShell();
>+    if (shell) {
>+      shell->NotifyFontSizeInflationEnabledIsDirty();
>+    }
>+  }
>+
>+  return rv;

This should live in PresShell::ContentInserted / ContentAppended / ContentRemoved instead (but you should first check that the parent is the document, otherwise it will be a performance problem!).

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -5368,70 +5368,18 @@ nsLayoutUtils::FontSizeInflationEnabled(
>   nsIPresShell* presShell = aPresContext->GetPresShell();
> 
>   if (!presShell ||
>       (presShell->FontSizeInflationEmPerLine() == 0 &&
>        presShell->FontSizeInflationMinTwips() == 0) ||
>        aPresContext->IsChrome()) {
>     return false;
>   }

These checks, except for the null-check on the presShell, should move into the pres shell method as well (and be cached).

>+void
> PresShell::SetupFontInflation()
> {
>   mFontSizeInflationEmPerLine = nsLayoutUtils::FontSizeInflationEmPerLine();
>   mFontSizeInflationMinTwips = nsLayoutUtils::FontSizeInflationMinTwips();
>   mFontSizeInflationLineThreshold = nsLayoutUtils::FontSizeInflationLineThreshold();
>   mFontSizeInflationForceEnabled = nsLayoutUtils::FontSizeInflationForceEnabled();
>   mFontSizeInflationDisabledInMasterProcess = nsLayoutUtils::FontSizeInflationDisabledInMasterProcess();
>+
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  if (os) {
>+    os->AddObserver(this, "font.size.inflation.forceEnabled", false);
>+    os->AddObserver(this, "font.size.inflation.disabledInMasterProcess", false);
>+  }
>+
>+  NotifyFontSizeInflationEnabledIsDirty();
>+}

You need to add preference observers, not observers to the observer service.  The preferences API also allows observers for an entire branch, e.g., you can also add an observer for "font.size.inflation." and get notified of changes to any of those preferences, which might be what you want here (especially after covering my comment above about moving the other checks).  I don't think there are any situations where the pref would change frequently, so I don't think the extra notifications for the one or two other prefs would be an issue.

>+  /**
>+   * Deregisters listeners listening for events that would affect font size
>+   * inflation.
>+   */
>+  void TearDownFontInflation();

deregister -> unregister

>diff --git a/view/src/nsViewManager.cpp b/view/src/nsViewManager.cpp
>--- a/view/src/nsViewManager.cpp
>+++ b/view/src/nsViewManager.cpp
>+
>+  // We need to recompute font inflation data now.
>+  if (mPresShell) {
>+    mPresShell->NotifyFontSizeInflationEnabledIsDirty();
>+  }
> }

If it was viewport width change you cared about, I'd think this should go in PresShell::ResizeReflowIgnoreOverride instead.  However, it's not clear to me why a viewport size change should reset this stuff, though -- but it looks like a screen size change should.  So I think it belongs in the function MaybeReflowForInflationScreenWidthChange in nsDOMWindowUtils.cpp.


(The other possibility if the observers are hard to get right -- is to just not mess with all the observer code, and just recompute whether font size inflation is enabled at the start of both PresShell::ProcessReflowCommands and PresShell::ResizeReflowIgnoreOverride.  That's much less computing of the value than we do now, probably simpler, and maybe safer as well.  Sorry for not suggesting that in comment 0.  I'm not really sure which approach I prefer, though.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> (The other possibility if the observers are hard to get right -- is to just
> not mess with all the observer code, and just recompute whether font size
> inflation is enabled at the start of both PresShell::ProcessReflowCommands
> and PresShell::ResizeReflowIgnoreOverride.  That's much less computing of
> the value than we do now, probably simpler, and maybe safer as well.  Sorry
> for not suggesting that in comment 0.  I'm not really sure which approach I
> prefer, though.)

Actually, this doesn't work if we ever need to know whether font inflation is enabled prior to reflow, which I seem to remember is the case (though worth checking).  (I think I went through this same chain of thought when I was filing the bug, and I should have explained the logic rather than just deleting that part of the comment.)

Comment 9

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8)
> Actually, this doesn't work if we ever need to know whether font inflation
> is enabled prior to reflow, which I seem to remember is the case (though
> worth checking).  (I think I went through this same chain of thought when I
> was filing the bug, and I should have explained the logic rather than just
> deleting that part of the comment.)

Yeah, so we check to see if font inflation is enabled in MaybeReflowForInflationScreenWidthChange, in order to see if a) inflation was enabled and b) our screen width has changed. If font inflation isn't enabled, then we don't perform the computation to see if our screen width had changed, which in turn triggers a reflow. So, if we were to change whether the inflation is enabled/disabled, and we don't recompute our cached value, it's possible that this code will have an incorrect state for mFontSizeInflationEnabled.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b862763 (v2)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> Comment on attachment 741034 [details] [diff] [review]
> b862763
> 
> This should live in PresShell::ContentInserted / ContentAppended /
> ContentRemoved instead (but you should first check that the parent is the
> document, otherwise it will be a performance problem!).

I've made this modification, as suggested, as well as the change to verify that the content node's parent is in the document in question.

> These checks, except for the null-check on the presShell, should move into
> the pres shell method as well (and be cached).

Done. The variables are already cached in the pres shell, but we also have static instances of them in nsLayoutUtils. The reason for this is so that the static instances in nsLayoutUtils can be updated immediately, when a preference changes, but we don't want the new values to go into effect until the user has reloaded the page. This was done to prevent a crash... I can't remember the exact bug number, though. 

> You need to add preference observers, not observers to the observer service.
> The preferences API also allows observers for an entire branch, e.g., you
> can also add an observer for "font.size.inflation." and get notified of
> changes to any of those preferences, which might be what you want here
> (especially after covering my comment above about moving the other checks). 
> I don't think there are any situations where the pref would change
> frequently, so I don't think the extra notifications for the one or two
> other prefs would be an issue.

Ah, ok, I didn't realize the difference immediately. I've made this change.

> deregister -> unregister

I've changed this, but the following stackexchange question seems to address the issue of "unregister" vs. "deregister" as a matter of opinion:
http://english.stackexchange.com/questions/25931/unregister-vs-deregister

Basically, the author of the accepted answer thinks, as I do, that "deregister" acts more as a verb (i.e. to "deregister a preference listener"), whereas "unregistered" is more of an adjective describing the current state of events (i.e. "the preference listener is now unregistered").

I'll change it, though, because I don't want this to mushroom into a bikeshed problem. ;)
  
> If it was viewport width change you cared about, I'd think this should go in
> PresShell::ResizeReflowIgnoreOverride instead.  However, it's not clear to
> me why a viewport size change should reset this stuff, though -- but it
> looks like a screen size change should.  So I think it belongs in the
> function MaybeReflowForInflationScreenWidthChange in nsDOMWindowUtils.cpp.

I've made the change to put it in MaybeReflowForInflationScreenWidthChange.

One thing I'm not sure on is the following code at the end of nsContentSink::ProcessMETATag():
>  // Because the viewport may have changed, we need to recompute font inflation
>  // settings.
>  nsCOMPtr<nsIPresShell> presShell = mDocument->GetShell();
>  if (presShell) {
>    presShell->NotifyFontSizeInflationEnabledIsDirty();
>  }

If I remove this code, it doesn't affect the reftests in layout/reftests/font-inflation, but it seems like it might be necessary to ensure that we have the correct font inflation state (enabled/disabled) if the <meta name="viewport"> tag were to change dynamically (I'm not sure if this is even possible). If the code should remain there, is it necessary to check to see if aContent is in mDocument, or is that an invariant that should hold throughout content processing/construction?

Comment 11

[Scott Johnson (:jwir3)]

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=de6683ef1995

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 741487 [details] [diff] [review]
b862763 (v2)

In MaybeReflowForInflationScreenWidthChange, the check presumably needs
to go earlier, *outside* the check for whether inflation is enabled,
because if whether inflation is enabled depends on the width, then it
could change either way.  Furthermore, if that's the case, then
MaybeReflowForInflationScreenWidthChange also needs to handle triggering
reflow when whether inflation is enabled changes.

The point of my suggestion about checking the container in the pres
shell notification methods was to make the check faster, since those
functions are called a lot.  What you want is probably:
  if (aContainer == aDocument &&
      aChild->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE)

>+  if (!nsCRT::strcmp(aTopic, "font.size.inflation.forceEnabled") ||
>+      !nsCRT::strcmp(aTopic, "font.size.inflation.disabledInMasterProcess")) {
>+    NotifyFontSizeInflationEnabledIsDirty();
>+    return NS_OK;
>+  }

First, this doesn't match how pref change observers are notified, so it
won't work.  (This tells me you either didn't research the patch
carefully enough or didn't test it carefully enough; you should have
done at least one better if not both.)

Second, you should be listening to the emPerLine and minTwips
preferences too.

>+void
>+nsIPresShell::NotifyFontSizeInflationEnabledIsDirty()
>+{
>+  mFontSizeInflationEnabledIsDirty = true;
>+}

This should be inline (i.e., in the header file).

Comment 13

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> First, this doesn't match how pref change observers are notified, so it
> won't work.  (This tells me you either didn't research the patch
> carefully enough or didn't test it carefully enough; you should have
> done at least one better if not both.)

Yes, you're right - I should never have posted this patch for review. I apologize for wasting your time. I realized after I pushed it to try server last night that there were a number of things wrong with it, and after looking over it again this morning, I should have cancelled the review.

My apologies.

Comment 14

[Scott Johnson (:jwir3)]

Another (greener) try run: 
https://tbpl.mozilla.org/?tree=Try&rev=d3546a0e764b

I'm still working on testing this to verify that it actually fixes the profile problems seen by jrmuizel.

Comment 15

[Scott Johnson (:jwir3)]

Attachment: b862763 (v3, WIP)

I'm posting the latest version of this, but I'm not requesting review just yet, because I'm still testing some aspects of it (see comment 14).

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> In MaybeReflowForInflationScreenWidthChange, the check presumably needs
> to go earlier, *outside* the check for whether inflation is enabled,
> because if whether inflation is enabled depends on the width, then it
> could change either way.  Furthermore, if that's the case, then
> MaybeReflowForInflationScreenWidthChange also needs to handle triggering
> reflow when whether inflation is enabled changes.
> 
> The point of my suggestion about checking the container in the pres
> shell notification methods was to make the check faster, since those
> functions are called a lot.  What you want is probably:
>   if (aContainer == aDocument &&
>       aChild->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE)

I did this, but it required a static_cast for both aContainer and aDocument to nsINode*. This probably isn't a big deal, but I didn't want to QI, as that would add unnecessary overhead.
> First, this doesn't match how pref change observers are notified, so it
> won't work.

> Second, you should be listening to the emPerLine and minTwips
> preferences too.

I've now verified that these match, with the following exception: the preference observer registers to listen to the entire "font.size.inflation." branch, whereas we only act on a subset of these (emPerLine, minTwips, forceEnabled, and disabledInMasterProcess).

The thing is, these observers might not even be necessary, because we don't re-read font size inflation settings until we reload the page (at which point, I think the pres shell is reconstructed, or am I wrong about this?). We did this because the font inflation code relies on frame state bits that are only set during frame construction. If we change the font inflation settings mid-way through, then on the next reflow, we get a frame tree that doesn't have these bits set, but the font inflation code expects them to be set.

> This should be inline (i.e., in the header file).

I've made it inline now.

I'm still a bit unsure about the nsContentSink code... it seems like it could be necessary in some cases where the viewport metadata changes, but I'm not entirely sure.

Comment 16

[Scott Johnson (:jwir3)]

Attachment: b862763 (v4)

I removed the code in nsContentSink, because I'm fairly certain this is only used during parsing, which IIUC, only happens on an initial load of a page (or, more likely, anytime the page is force-reloaded, but in any case, the pres shell will be reconstructed, so the font size inflation will already be queued for a refresh).

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 744144 [details] [diff] [review]
b862763 (v4)

nsDOMWindowUtils.cpp:

  Could you make this also do the reflow if whether inflation was 
  enabled changes in *either* direction (in addition to reflowing if the
  screen width changes).  This requires removing the outermost check of
  presShell->FontSizeInflationEnabled(), and only checking 
  FontSizeInflationMinTwips() for the screen width change case (and not
  the enabled-change case).

  (The old code here doesn't account for the fact that a screen width
  change being able to change whether font inflation is enabled at all
  based on the page having a <meta viewport>.)

  That said, this could be done in a followup patch if you want.  (And 
  if you think it needs review, probably should be.)

nsPresShell.cpp:

  The checks in ContentAppended/ContentInserted/ContentRemoved don't 
  seem quite right to me, especially given the comments in 
  nsIMutationObserver.h ***which are different between the methods*** 
  for what happens when the parent is a document (in some cases 
  aContainer is null, in others, the document).

  Could you:
   (a) check that the comments in nsIMutationObserver are really correct
   based on looking at the code (and if they're not, fix them, and have
   a content peer review), and

   (b) fix the code here to use the correct quick check for "node being
   notified on is a child of the document"

  In RecomputeFontSizeInflationEnabled:

  >+  if (!nsLayoutUtils::FontSizeInflationForceEnabled()) {

  This should still check the pres shell method (cached) rather than
  the nsLayoutUtils method.

  >+  if (vInf.GetDefaultZoom() >= 1.0 || vInf.IsAutoSizeEnabled()) {

  Given that you're moving code, you should fix the indent on this
  line (2 more spaces).

r=dbaron with that

Comment 18

[Scott Johnson (:jwir3)]

Attachment: b862763-followup

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #17)
> Comment on attachment 744144 [details] [diff] [review]
> b862763 (v4)
> 
> nsDOMWindowUtils.cpp:
> 
>   Could you make this also do the reflow if whether inflation was 
>   enabled changes in *either* direction (in addition to reflowing if the
>   screen width changes).  This requires removing the outermost check of
>   presShell->FontSizeInflationEnabled(), and only checking 
>   FontSizeInflationMinTwips() for the screen width change case (and not
>   the enabled-change case).
> 
>   (The old code here doesn't account for the fact that a screen width
>   change being able to change whether font inflation is enabled at all
>   based on the page having a <meta viewport>.)
> 
>   That said, this could be done in a followup patch if you want.  (And 
>   if you think it needs review, probably should be.)

Posting a followup patch now. I do believe it needs review.

> nsPresShell.cpp:
> 
>   The checks in ContentAppended/ContentInserted/ContentRemoved don't 
>   seem quite right to me, especially given the comments in 
>   nsIMutationObserver.h ***which are different between the methods*** 
>   for what happens when the parent is a document (in some cases 
>   aContainer is null, in others, the document).
> 
>   Could you:
>    (a) check that the comments in nsIMutationObserver are really correct
>    based on looking at the code (and if they're not, fix them, and have
>    a content peer review), and

Yes, they are correct. However, I noticed that there is a line in PresShell::ContentRemoved that calls EventStateManager()->ContentRemoved(), which in turn calls nsFocusManager::ContentRemoved(), which assumes that aDocument is non-null. This is ok in the pres shell case, because we assert at the beginning of the function that mDocument == aDocument, and mDocument can't be null, unless we're in initialization, so I think this is ok. 

I added a comment to explain this situation and make it more explicit.

>   In RecomputeFontSizeInflationEnabled:
> 
>   >+  if (!nsLayoutUtils::FontSizeInflationForceEnabled()) {
> 
>   This should still check the pres shell method (cached) rather than
>   the nsLayoutUtils method.

Fixed.

>   >+  if (vInf.GetDefaultZoom() >= 1.0 || vInf.IsAutoSizeEnabled()) {
> 
>   Given that you're moving code, you should fix the indent on this
>   line (2 more spaces).

Fixed. Thanks for pointing that out. I didn't see it. :)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 749106 [details] [diff] [review]
b862763-followup

> static void
> MaybeReflowForInflationScreenWidthChange(nsPresContext *aPresContext)
> {
>   if (aPresContext) {
>     nsIPresShell* presShell = aPresContext->GetPresShell();
>     bool fontInflationWasEnabled = presShell->FontSizeInflationEnabled();
>     presShell->NotifyFontSizeInflationEnabledIsDirty();
>+    bool changed;

Initialize changed to false here; otherwise with the new code flow it could end up being read without being initialized.

r=dbaron with that

Comment 20

[Scott Johnson (:jwir3)]

Try run for both of these patches:
https://tbpl.mozilla.org/?tree=Try&rev=6ae32f82baf8

Comment 21

[Scott Johnson (:jwir3)]

Inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a522f886a854
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef898d951ef

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a522f886a854
https://hg.mozilla.org/mozilla-central/rev/5ef898d951ef

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b00d3311db25
https://hg.mozilla.org/releases/mozilla-b2g18/rev/433c7839607d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eda25a4c8d2f

https://tbpl.mozilla.org/php/getParsedLog.php?id=23060188&tree=Mozilla-B2g18

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/eca90a8f5eee
https://hg.mozilla.org/releases/mozilla-b2g18/rev/321d5b4db55a

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like this change resulted in a ~14% Tp4 (no chrome) improvement for Android:
http://graphs.mozilla.org/graph.html#tests=[[84,63,20]]&sel=1368716923082.4492,1368726290749.4385&displayrange=7&datatype=running

(zoom out to see the new trend)