Closed Bug 1351383 Opened 7 years ago Closed 7 years ago

Create a telemetry probe to measure usage of css box align properties on display:block containers

Categories

(Core :: Layout: Block and Inline, enhancement)

Product:
Core
Component:
Layout: Block and Inline
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1351383 Part 1: Define a new histogram to measure usage of css box align props in block containers.
59 bytes, text/x-review-board-request
Details
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
We need to see if there will be breakage when we implement css box align for block containers in bug 1105571.
Attachment #8852100 - Flags: feedback?(benjamin)
'justify-content' doesn't apply to blocks AFAICT:
https://drafts.csswg.org/css-align-3/#distribution-block

'justify-items' is merely a placeholder for 'justify-self:auto' children,
so that's probably not that interesting either.  What's interesting is if
there are any block-level children with a computed 'justify-self' that is
not 'normal' and not 'start' (which is what 'normal' "behaves as").
https://drafts.csswg.org/css-align-3/#justify-block

You can probably pick that up in nsBlockFrame::ReflowBlockFrame, and for
float children in nsBlockFrame::ReflowFloat.

(It seems 'justify-self' doesn't apply at all to inline children.)
Now I see "This property does not apply to floats." at the end of 6.1.1
so we can skip that part.
Comment on attachment 8852100 [details]
Bug 1351383 Part 1: Define a new histogram to measure usage of css box align props in block containers.

https://reviewboard.mozilla.org/r/124338/#review127306

::: toolkit/components/telemetry/Histograms.json:210
(Diff revision 1)
>      "alert_emails": ["rvitillo@mozilla.com"],
>      "expires_in_version": "35",
>      "kind": "boolean",
>      "description": "blocklist.xml has been loaded synchronously *** No longer needed (bug 1156565). Delete histogram and accumulation code! ***"
>    },
> +  "BOX_ALIGN_PROPS_IN_BLOCKS_FLAG": {

Do you need this to be opt-out for the release population to get the data you need? In particular beta/prerelease doesn't have very good coverage of a bunch of geographies.

data-r=me either way
Attachment #8852100 - Flags: review+
Attachment #8852100 - Flags: feedback?(benjamin)
Attachment #8852101 - Flags: review?(dholbert)
Comment on attachment 8852101 [details]
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.

https://reviewboard.mozilla.org/r/124340/#review129026

::: layout/generic/nsBlockFrame.cpp:1516
(Diff revision 2)
> +  // Bug 1351383: Collect data for the BOX_ALIGN_PROPS_IN_BLOCKS_FLAG probe.
> +  auto IsBoxAlignProperty = [](uint16_t value)->bool {
> +    return (value == NS_STYLE_ALIGN_CENTER ||
> +            value == NS_STYLE_ALIGN_START ||

Two things:
(1) This seems misnamed.  (Every property whose value you pass to this is a box-align-property.) Perhaps a more accurate name would be "IsNonDefaultBoxAlignVal"?  BUT, before you fix that, read on:

(2) Maybe you should really be checking against the default value, rather than against an enumeration of all non-defaults?  For each of the properties here, the default is "normal"*, so it seems like this could just check for != NS_STYLE_ALIGN_NORMAL.  (Or perhaps better, rename the method to "IsNormal" and check for == NS_STYLE_ALIGN_NORMAL.)

* For justify-items & justify-self, the initial value is more complicated than 'normal', but it works out to 'normal' if everything is unset, so I think a "normal" comparison is good enough. (Please sanity-check me on this, though.)

::: layout/generic/nsBlockFrame.cpp:1530
(Diff revision 2)
> +            value == NS_STYLE_ALIGN_SPACE_BETWEEN ||
> +            value == NS_STYLE_ALIGN_SPACE_EVENLY ||
> +            value == NS_STYLE_ALIGN_SPACE_AROUND);
> +  };
> +
> +  // First check this frame's properties.

This is a little too vague -- you're checking *different* properties on the block vs. on its children, so it'd be good to clarify why.

How about:
 // First check this frame for non-default values of the css-align properties
 // that apply to block containers.

::: layout/generic/nsBlockFrame.cpp:1531
(Diff revision 2)
> +  // We look for any non-default value for justify-items, but don't explicitly
> +  // check to see if any block-level children have justify-self:auto.

This side-comment is a bit roundabout. Perhaps replace/clarify with something like:

  // Note: we check here for non-default "justify-items", though technically
  // that'd only affect rendering if some child has "justify-self:auto".
  // (It's safe to assume that's likely, since it's the default value that
  // a child would have.)

::: layout/generic/nsBlockFrame.cpp:1533
(Diff revision 2)
> +  if (IsBoxAlignProperty(StylePosition()->mJustifyContent) ||
> +      IsBoxAlignProperty(StylePosition()->mAlignContent) ||
> +      IsBoxAlignProperty(StylePosition()->ComputedJustifyItems(

The StyleWhatever() accessors aren't free.  If you have a ReflowInput object available for a frame (and you do here), you should always favor using its cached pointers to the style structs, rather than calling StyleWhatever() yourself.

So, you should probably do:
 const nsStylePosition* stylePos = areflowinput.mStylePosition;

...and then s/StylePosition()/stylePos/ for this section.

::: layout/generic/nsBlockFrame.cpp:1535
(Diff revision 2)
> +      IsBoxAlignProperty(StylePosition()->ComputedJustifyItems(
> +        StyleContext()->GetParent()))) {

As of bug 1322570, "StyleContext()->GetParent()" will fatally assert in Stylo.  So you can't do that here, I think.

You *could* use StyleContext()->GetParentAllowServo() which bypasses the assertion -- but that's still kind of bad, because (a) it's a hack, and (b) ComputedJustifyItems then has to walk all the way up the style context tree, which is expensive.

I'd say you should just pass "nullptr" into ComputedJustifyItems here, and add a comment explaining that we're trading off a little bit of edge-case-correctness in order to reduce the performance hit from this measurement.  (The ComputedJustifyItems argument only matters for resolving the "justify-items" default value -- and that default value is *basically* equivalent to 'normal', except in an edge case which I don't expect web developers to have accidentally stumbled into.)

::: layout/generic/nsBlockFrame.cpp:1542
(Diff revision 2)
> +    mozilla::Telemetry::Accumulate(
> +      mozilla::Telemetry::BOX_ALIGN_PROPS_IN_BLOCKS_FLAG, true);
> +  } else {
> +    // If not already flagged by the parent, now check justify-self of the
> +    // block-level child frames.
> +    for (auto iter = mLines.begin(); iter != mLines.end(); iter++) {

I can't find any similar instances of this for-loop construct in this file. It looks like we typically use LinesBegin() and LinesEnd(), so might as well stick with that here for consistency & brevity.  Also, you should use "++iter" rather than "iter++", since the latter can be slower due to having to return the old value of the iterator.

So, let's change this loop to the following (matching syntax I found elsewhere in this file) for consistency:
    for (nsBlockFrame::LineIterator line = LinesBegin();
         line != LinesEnd(); ++line) {

::: layout/generic/nsBlockFrame.cpp:1545
(Diff revision 2)
> +        mozilla::Telemetry::Accumulate(
> +          mozilla::Telemetry::BOX_ALIGN_PROPS_IN_BLOCKS_FLAG, true);
> +        break;

A few general concerns:
 (1) If we relayout the same frame over and over (and hit this line over and over for that frame), will this produce an artificially high count?  (I really don't know -- I'm new to our Telemetry, but I don't see how that would be prevented right now.)  If we could make this a per-document flag, that would be great -- but I don't know if that's possible.

 (2) You can drop the "mozilla::" prefix in both lines here -- this file has "using namespace mozilla".

 (3) AFAICT, this will ride the trains all the way to release -- there's no mechanism to turn it off there right now.  Is that intended?  (This is kinda bad, since this measurement probably comes with a small perf hit from walking our lines list an extra time.)  If we know ahead of time that we don't intend this to hit release [not sure], we should bake that into this via an about:config pref, or at least file a bug on ripping it out and add a code-comment here about when we expect to rip it out.
Attachment #8852101 - Flags: review?(dholbert) → review-
Blocks: 1358299
Comment on attachment 8852101 [details]
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.

https://reviewboard.mozilla.org/r/124340/#review129026

> A few general concerns:
>  (1) If we relayout the same frame over and over (and hit this line over and over for that frame), will this produce an artificially high count?  (I really don't know -- I'm new to our Telemetry, but I don't see how that would be prevented right now.)  If we could make this a per-document flag, that would be great -- but I don't know if that's possible.
> 
>  (2) You can drop the "mozilla::" prefix in both lines here -- this file has "using namespace mozilla".
> 
>  (3) AFAICT, this will ride the trains all the way to release -- there's no mechanism to turn it off there right now.  Is that intended?  (This is kinda bad, since this measurement probably comes with a small perf hit from walking our lines list an extra time.)  If we know ahead of time that we don't intend this to hit release [not sure], we should bake that into this via an about:config pref, or at least file a bug on ripping it out and add a code-comment here about when we expect to rip it out.

1) This is an issue, but my understanding is that this build will primarily be used internally on top sites, checking for non-zero values in constrained tests. So we'll know that *url-X* triggers the probe but *url-Y* doesn't. In the wild, it has limited usefulness for us.
2) Thank you. I removed "mozilla::".
3) Yes, it will ride the trains unless we take steps. I'll check with Jet if he even wants this landed, or just built and handed off to internal testing teams. For the moment, I logged a bug to remove the code and histogram, presuming this does land. It seems there isn't a runtime method for determining whether a probe is "opted". If there was, we could make this probe opt-in and its presence in 56 would be relatively harmless since users likely wouldn't turn the probe on.
Attachment #8860197 - Attachment is obsolete: true
Attachment #8860198 - Attachment is obsolete: true
(In reply to Brad Werth [:bradwerth] from comment #8)

> 3) Yes, it will ride the trains unless we take steps. I'll check with Jet if
> he even wants this landed, or just built and handed off to internal testing
> teams. For the moment, I logged a bug to remove the code and histogram,
> presuming this does land. It seems there isn't a runtime method for
> determining whether a probe is "opted". If there was, we could make this
> probe opt-in and its presence in 56 would be relatively harmless since users
> likely wouldn't turn the probe on.

I learned from telemetry team that there is a global parameter for telemetry that I can check, so I enclosed all the code within that check and switched the test to opt-in for release builds.
Comment on attachment 8852101 [details]
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.

https://reviewboard.mozilla.org/r/124340/#review135930

This is nearly r+, but I'm keeping it at r- for now since it's probably worth a final sanity-check of the AddBoolVarCache & #ifdeffing that I'm suggesting here. I promise swift final-review.

::: layout/generic/nsBlockFrame.cpp:1516
(Diff revision 5)
>                     delta, perLineDelta, numLines, ectc - ctc);
>      printf("%s\n", buf);
>    }
>  #endif
>  
> +  // Bug 1358299 START: Remove this code after the 56 merge date.

It's not clear to me (from the bug & this code-comment & comment 8 part 3) whether this is intended to ride the trains to beta & release.  If this is mostly for internal testing, then perhaps not?

If you want this to be "early-beta or earlier" (which might be a good idea to be on the safe side & avoid regressing performance in release builds), you should consider wrapping this whole chunk in:
  #ifdef EARLY_BETA_OR_EARLIER

::: layout/generic/nsBlockFrame.cpp:1516
(Diff revision 5)
> +  // Bug 1358299 START: Remove this code after the 56 merge date.
> +  if (Preferences::GetBool("toolkit.telemetry.enabled")) {
> +    // Bug 1351383: Collect data for the BOX_ALIGN_PROPS_IN_BLOCKS_FLAG probe.
> +    auto IsStyleNormal = [](uint16_t value)->bool {

Probably no need to mention the second bug number (bug 1351383, for this very bug page).  It'll already be encoded via the "hg blame" for this section of code, so it's kinda redundant to explicitly mention it in a comment too.  And generally, bug numbers in the codebase are hints that "We need to follow up on this, as part of Bug NNN" (where Bug NNN is some bug that is still open).  So for all those reasons, we don't typically include links to closed bugs in the codebase unless there's a special reason that we need to. [and bug 1351383, this bug, will be a closed bug as soon as this patch lands]

::: layout/generic/nsBlockFrame.cpp:1517
(Diff revision 5)
>      printf("%s\n", buf);
>    }
>  #endif
>  
> +  // Bug 1358299 START: Remove this code after the 56 merge date.
> +  if (Preferences::GetBool("toolkit.telemetry.enabled")) {

Unfortunately, "Preferences::GetBool()" can be expensive.  You pretty much never want to call that directly -- at least not on "hot" codepaths like during reflow.

Instead, please use two static local variables together with AddBoolVarCache, like we do here for example:
https://dxr.mozilla.org/mozilla-central/rev/e17cbb839dd225a2da7e5d5bec43cf94e11749d8/layout/base/nsLayoutUtils.cpp#644-653

(That wires up the boolean to be an up-to-date cache of the about:config variable, which gets updated if/when the pref changes [which is a rare thing].)

::: layout/generic/nsBlockFrame.cpp:1535
(Diff revision 5)
> +      Telemetry::Accumulate(
> +        Telemetry::BOX_ALIGN_PROPS_IN_BLOCKS_FLAG, true);

No need to wrap here. Let's just make this a one-liner.
Attachment #8852101 - Flags: review?(dholbert) → review-
Comment on attachment 8852101 [details]
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.

https://reviewboard.mozilla.org/r/124340/#review135930

> It's not clear to me (from the bug & this code-comment & comment 8 part 3) whether this is intended to ride the trains to beta & release.  If this is mostly for internal testing, then perhaps not?
> 
> If you want this to be "early-beta or earlier" (which might be a good idea to be on the safe side & avoid regressing performance in release builds), you should consider wrapping this whole chunk in:
>   #ifdef EARLY_BETA_OR_EARLIER

Had my conversation with Jet and he suggested this NOT go to release, so I've made the changes you request here, and have turned the probe back to opt-out. I had to make the IsStyleNormal function into IsStyleNormalOrAuto to avoid false positives with children. I didn't think through why auto was coming up so much on block children, but it was happening essentially everywhere, and it's not something this probe wants to measure.
(In reply to Brad Werth [:bradwerth] from comment #20)
> Had my conversation with Jet and he suggested this NOT go to release, so
> I've made the changes you request here, and have turned the probe back to
> opt-out. I had to make the IsStyleNormal function into IsStyleNormalOrAuto
> to avoid false positives with children.

That makes sense. mJustifySelf is NS_STYLE_ALIGN_AUTO by default (which is hand-waved away via the UsedJustifySelf() API, but we don't need to bother with that here.)
Comment on attachment 8852101 [details]
Bug 1351383 Part 2: Collect telemetry for nsBlockFrame and children with css box align styles.

https://reviewboard.mozilla.org/r/124340/#review137052

r=me
Attachment #8852101 - Flags: review?(dholbert) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/443cbeb485e5
Part 1: Define a new histogram to measure usage of css box align props in block containers. r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/9fd99f28498d
Part 2: Collect telemetry for nsBlockFrame and children with css box align styles. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/443cbeb485e5
https://hg.mozilla.org/mozilla-central/rev/9fd99f28498d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: