Convert NS_STYLE_DISPLAY_* to an enum class, avoid linear search by display in the frame constructor.

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Now I'm here, I'm not sure it was worth the churn, but anyway. Sorry for the massive commit message, Boris.

Also, I'll try to squash them appropriately, but thought they might be easier to review this way.
Assignee

Updated

3 years ago
Blocks: 1277133

Comment 6

3 years ago
mozreview-review
Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73060

::: layout/style/nsStyleStruct.h:2993
(Diff revision 1)
> -    return mozilla::StyleDisplay::Ruby == aDisplay ||
> -           mozilla::StyleDisplay::RubyBase == aDisplay ||
> +    return aDisplay >= mozilla::StyleDisplay::Ruby &&
> +           aDisplay <= mozilla::StyleDisplay::RubyTextContainer;
> -           mozilla::StyleDisplay::RubyBaseContainer == aDisplay ||
> -           mozilla::StyleDisplay::RubyText == aDisplay ||
> -           mozilla::StyleDisplay::RubyTextContainer == aDisplay;

I expect compiler be able to work out this kind of simple optimization. I don't think we should replace those equal comparisons with a range comparison, unless you can ensure in compile time that the range includes and only includes these values. Otherwise I think this is a regression on type safety.
Assignee

Comment 7

3 years ago
mozreview-review-reply
Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73060

> I expect compiler be able to work out this kind of simple optimization. I don't think we should replace those equal comparisons with a range comparison, unless you can ensure in compile time that the range includes and only includes these values. Otherwise I think this is a regression on type safety.

That's fair, I'll revert them.
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8786234 - Attachment is obsolete: true
Attachment #8786234 - Flags: review?(cam)
Assignee

Updated

3 years ago
Attachment #8786235 - Attachment is obsolete: true
Attachment #8786235 - Flags: review?(cam)
Assignee

Updated

3 years ago
Attachment #8786236 - Attachment is obsolete: true
Attachment #8786236 - Flags: review?(cam)
Comment hidden (mozreview-request)
Mozreview isn't funny posting just one commit, sigh.

Comment 13

3 years ago
mozreview-review
Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73216

::: layout/base/nsCSSFrameConstructor.h:735
(Diff revision 2)
>      const FrameConstructionData mData;
>    };
>  
> +  struct FrameConstructionDataByDisplay {
> +#ifdef DEBUG
> +    const mozilla::StyleDisplay mDisplay;

I assume for commit purposes this is being squashed with the commits that actually define the enum type and whatnot, right?  Want to avoid bisect footguns.

::: layout/base/nsCSSFrameConstructor.cpp:4316
(Diff revision 2)
>  
>  #define SIMPLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SIMPLE_XUL_FCDATA(_func) }
>  #define SCROLLABLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \

s/_int/_display/ here, please.

::: layout/base/nsCSSFrameConstructor.cpp:4318
(Diff revision 2)
>    { &nsGkAtoms::_tag, SIMPLE_XUL_FCDATA(_func) }
>  #define SCROLLABLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> -  { _int, SIMPLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_XUL_INT_CREATE(_int, _func)                          \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \
> +  FCDATA_FOR_DISPLAY(_int, SIMPLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_XUL_DISPLAY_CREATE(_int, _func)                          \

And here too, s/_int/_display/

::: layout/base/nsCSSFrameConstructor.cpp:4320
(Diff revision 2)
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> -  { _int, SIMPLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_XUL_INT_CREATE(_int, _func)                          \
> -  { _int, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_ABSPOS_CONTAINER_XUL_INT_CREATE(_int, _func)         \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \
> +  FCDATA_FOR_DISPLAY(_int, SIMPLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_XUL_DISPLAY_CREATE(_int, _func)                          \
> +  FCDATA_FOR_DISPLAY(_int, SCROLLABLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_ABSPOS_CONTAINER_XUL_DISPLAY_CREATE(_int, _func)         \

And here.

::: layout/base/nsCSSFrameConstructor.cpp:4530
(Diff revision 2)
>  
> -  // Processing by display here:
> -  return FindDataByInt(aDisplay->mDisplay, aElement, aStyleContext,
> -                       sXULDisplayData, ArrayLength(sXULDisplayData));
> +  if (aDisplay->mDisplay < StyleDisplay::Box) {
> +    return nullptr;
> +  }
> +
> +  MOZ_ASSERT(aDisplay->mDisplay <= StyleDisplay::Popup,

Please add a comment in nsStyleConsts.h about how if values are inserted in the XUL bits or added at the end this code needs to be updated.

::: layout/base/nsCSSFrameConstructor.cpp:4742
(Diff revision 2)
>        return &sNonScrollableGridData;
>      }
>    }
>  
> -  static const FrameConstructionDataByInt sDisplayData[] = {
> -    // To keep the hash table small don't add inline frames (they're
> +  // To keep the hash table small don't add inline frames (they're

Please keep this comment next to the StyleDisplay::Inline bit.

::: layout/base/nsCSSFrameConstructor.cpp:4840
(Diff revision 2)
>  
> -  return FindDataByInt(aDisplay->mDisplay,
> -                       aElement, aStyleContext, sDisplayData,
> -                       ArrayLength(sDisplayData));
> +  const FrameConstructionDataByDisplay& data =
> +    sDisplayData[size_t(aDisplay->mDisplay)];
> +
> +  MOZ_ASSERT(data.mDisplay == aDisplay->mDisplay,
> +             "Someone messed the order in the display values");

"messed up"

::: layout/style/nsStyleConsts.h:490
(Diff revision 2)
>             NS_STYLE_WRITING_MODE_SIDEWAYS_MASK)
>  
>  // See nsStyleDisplay
> +//
> +// NOTE: Order is important! If you change it, make sure to take a look at
> +// the FrameConstructorDataByDisplay stuff, and ensure it's still correct!

And XUL stuff, right?

r=me
Attachment #8786237 - Flags: review?(bzbarsky) → review+

Comment 14

3 years ago
mozreview-review
Comment on attachment 8786243 [details]
Bug 1299066: Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor

https://reviewboard.mozilla.org/r/75244/#review73864

::: layout/style/nsStyleConsts.h:518
(Diff revision 1)
> -#define NS_STYLE_DISPLAY_POPUP                  27
> -#define NS_STYLE_DISPLAY_GROUPBOX               28
> +  Contents,
> +  WebkitBox,
> +  WebkitInlineBox,
> +  Box,
> +  InlineBox,
> +#ifdef MOZ_XUL // XXX why not removing this?

I'm not sure what the state of --disable-xul builds are, but I just tried and the build fails pretty early on.  Certainly they're not going to be useful for Firefox.

In bug 540222 comment 5 Mats suggests that the MOZ_XUL blocks are still good for documentation, but I'm not sure it's really worth it.
Attachment #8786243 - Flags: review?(cam) → review+

Comment 15

3 years ago
mozreview-review
Comment on attachment 8786244 [details]
Bug 1299066: Rename NS_STYLE_DISPLAY_* to use the new enum class.

https://reviewboard.mozilla.org/r/75246/#review73866

::: layout/style/nsRuleNode.cpp:202
(Diff revision 1)
>  nsRuleNode::EnsureBlockDisplay(uint8_t& display,
>                                 bool aConvertListItem /* = false */)
>  {
>    // see if the display value is already a block
>    switch (display) {
> -  case NS_STYLE_DISPLAY_LIST_ITEM :
> +  case StyleDisplay::ListItem :

Nit: feel free to drop the spaces before the ":"s while touching these lines.

::: layout/style/nsRuleNode.cpp:6253
(Diff revision 1)
>        // XXX We may also need to promote ruby display vals; see bug 1179349.
>  
>        // It's okay to cache this change in the rule tree for the same
>        // reasons as floats in the previous condition.
> -      if (display->mDisplay == NS_STYLE_DISPLAY_INLINE) {
> -          display->mDisplay = NS_STYLE_DISPLAY_INLINE_BLOCK;
> +      if (display->mDisplay == StyleDisplay::Inline) {
> +          display->mDisplay = StyleDisplay::InlineBlock;

This line has confusing indentation.  Fix it up while you're here?
Attachment #8786244 - Flags: review?(cam) → review+

Comment 16

3 years ago
mozreview-review
Comment on attachment 8786245 [details]
Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

https://reviewboard.mozilla.org/r/75248/#review73876

Let's not add "mozilla::" to the .cpp files.  Most .cpp files have a |using namespace mozilla;| at the top.  For those that don't, and we would otherwise need mozilla::StyleDisplay, just add the using statement.

r=me with these comments addressed.

::: dom/base/nsRange.cpp:3504
(Diff revision 1)
>              !IsLastNonemptyRowGroupOfTable(f->GetParent())) {
>            result.Append('\n');
>          }
>          break;
> +      default:
> +        ; // Nothing

Nit: might prefer:

  default:
    break;  // do nothing

just so we don't leave a fall through in case we add a new case below it later.

::: layout/generic/nsFrame.cpp:10788
(Diff revision 1)
>      // This array must exactly match the NS_STYLE_DISPLAY constants.
>      const char *const displayTypes[] = {
>        "none", "block", "inline", "inline-block", "list-item", "marker",
>        "run-in", "compact", "table", "inline-table", "table-row-group",
>        "table-column", "table-column-group", "table-header-group",
>        "table-footer-group", "table-row", "table-cell", "table-caption",
>        "box", "inline-box",
>  #ifdef MOZ_XUL
>        "grid", "inline-grid", "grid-group", "grid-line", "stack",
>        "inline-stack", "deck", "popup", "groupbox",
>  #endif
>      };

I guess this array needs to be updated with your previous patch.

::: layout/style/nsRuleNode.cpp:280
(Diff revision 1)
>        break;
>      case StyleDisplay::Stack:
>        display = StyleDisplay::InlineStack;
>        break;
> +    default:
> +      ; // Nothing

As above.

::: layout/style/nsRuleNode.cpp:6213
(Diff revision 1)
>          case StyleDisplay::InlineFlex:
>            display->mDisplay = StyleDisplay::Flex;
>            conditions.SetUncacheable();
>            break;
> +        default:
> +          ; // Nothing

And here.

::: layout/style/nsStyleStruct.h:2830
(Diff revision 1)
>                                  //         and float:left/right; otherwise equal
>                                  //         to mDisplay

Re-align these two comment lines?
Attachment #8786245 - Flags: review?(cam) → review+
Assignee

Comment 17

3 years ago
mozreview-review-reply
Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73216

> I assume for commit purposes this is being squashed with the commits that actually define the enum type and whatnot, right?  Want to avoid bisect footguns.

Sure

> And XUL stuff, right?

Sure, the XUL array is also a FrameConstructorDataByDisplay array, but I'll clarify.
Assignee

Comment 18

3 years ago
mozreview-review-reply
Comment on attachment 8786245 [details]
Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

https://reviewboard.mozilla.org/r/75248/#review73876

Unfortunately we can't on some of the files (the frame ones concretely), because some compilers on try for some reason get confused with the `StyleDisplay` method :/

> I guess this array needs to be updated with your previous patch.

It was already broken, but yeah, I fixed it anyway.
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8786244 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8786245 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8786237 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8786243 - Flags: review?(bzbarsky)

Comment 20

3 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c212e496d0ce
Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor r=heycam,bz

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c212e496d0ce
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.