1395586 - Add accessors to trivial nsStyleDisplay members, and make them private.

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

This will allow me to pack them up using bitfields more trivially, and I have the patch written already so...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395586: Add accessors and privatize trivial nsStyleDisplay members.

While at it, also reorder some of them to save some space.

The idea is to autogenerate code for these, and to pack them using bitfields. It
still needs some more code, but I think I'll try to get something next week.

This is preliminary anyway, but I wanted to land it since they're huge changes.

Review commit: https://reviewboard.mozilla.org/r/174976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174976/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395586: layout/style build fixes (to be squashed).

Review commit: https://reviewboard.mozilla.org/r/174978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174978/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395586: Other build fixes (to be squashed too).

Review commit: https://reviewboard.mozilla.org/r/174980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174980/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903190 [details]
Bug 1395586: Add accessors and privatize trivial nsStyleDisplay members.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174976/diff/1-2/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903191 [details]
Bug 1395586: layout/style build fixes (to be squashed).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174978/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903192 [details]
Bug 1395586: Other build fixes (to be squashed too).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174980/diff/1-2/

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8903190 [details]
Bug 1395586: Add accessors and privatize trivial nsStyleDisplay members.

https://reviewboard.mozilla.org/r/174976/#review180298

::: commit-message-735d2:5
(Diff revision 2)
> +The idea is to autogenerate code for these, and to pack them using bitfields. It
> +still needs some more code, but I think I'll try to get something next week.

I don't see why it is necessary to hide them behind private in order to pack them using bitfields.

C++ supports bitfields directly, no?

You may hit warning on GCC stating that a bitfield is too small for some enum class... but that warning is just too silly that we should probably disable.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> Comment on attachment 8903190 [details]
> Bug 1395586: Add accessors and privatize trivial nsStyleDisplay members.
> 
> https://reviewboard.mozilla.org/r/174976/#review180298
> 
> ::: commit-message-735d2:5
> (Diff revision 2)
> > +The idea is to autogenerate code for these, and to pack them using bitfields. It
> > +still needs some more code, but I think I'll try to get something next week.
> 
> I don't see why it is necessary to hide them behind private in order to pack
> them using bitfields.
> 
> C++ supports bitfields directly, no?

It does. But basically, I want to add setters to ensure we don't mess up the field's width and clamp incorrectly.

Basically, I want to end up with something like:

void SetDisplay(StyleDisplay aDisplay) {
  mDisplay = aDisplay;
  MOZ_ASSERT(Display() == aDisplay);
}

Comment 9

[Cameron McCormack (:heycam)]

Before going ahead with this can you give me an idea of (a) how much space we'll be able to save once we pack these enums together tightly, and (b) how you plan to do the field width checking?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: pack.patch

(In reply to Cameron McCormack (:heycam) from comment #9)
> (a) how much space we'll be able to save once we pack these enums together tightly

After a quick patch (attached below, some of them may be able to be packed better), sizeof(nsStyleDisplay) gets reduced by two words on clang (408 -> 392).

I think we may be able to come up with other space optimizations for this struct, specially re. the AutoArrays for transitions and animations (can we move them to another struct?). That can probably also be important.

> (b) how do you plan to do the field width checking?

The same way as in that patch, basically, asserting that the getter returns the value we've set.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8903190 [details]
Bug 1395586: Add accessors and privatize trivial nsStyleDisplay members.

https://reviewboard.mozilla.org/r/174976/#review180892

::: commit-message-735d2:5
(Diff revision 2)
> +The idea is to autogenerate code for these, and to pack them using bitfields. It
> +still needs some more code, but I think I'll try to get something next week.

Please update this with a final message that describes the change without mentioning what the patch author will be doing the following week. :-)

::: layout/style/nsStyleStruct.h:2670
(Diff revision 2)
>  
> +// Please keep the fields that use this macro private, the idea is for them to
> +// be packed in the near future.
> +//
> +// TODO(emilio): It'd be nice for style structs to be auto-generated.
> +#define PRIVATE_MEMBER(type_, name_)       \

The important aspect (in the near future, anyway) seems to be that it's an enum-typed field and that it can be packed.  So how about we call the macro something like PACKED_ENUM instead?

::: layout/style/nsStyleStruct.h:2672
(Diff revision 2)
> +// be packed in the near future.
> +//
> +// TODO(emilio): It'd be nice for style structs to be auto-generated.
> +#define PRIVATE_MEMBER(type_, name_)       \
> +  private:                                 \
> +  type_ m##name_;                          \

Nit: indent this?

::: layout/style/nsStyleStruct.h:2675
(Diff revision 2)
> +  type_ name_() const { return m##name_; } \
> +  void Set##name_(type_ a##name_) {        \
> +    m##name_ = a##name_;                   \
> +  }

Nit: indent this too?

::: layout/style/nsStyleStruct.h:2786
(Diff revision 2)
> +  //
> +  // FIXME(emilio): This takes lots of space in the common no transitions and
> +  // no animations case. Can we do better?

Move this comment up to the block of transitions-related properties?

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8903191 [details]
Bug 1395586: layout/style build fixes (to be squashed).

https://reviewboard.mozilla.org/r/174978/#review180900

It feels a bit weird to me to be setting back style struct values when we have eCSSUnit_Null, but since we started our local variable out with the current value it's OK.  But I guess it's not a big deal.

::: layout/style/GeckoStyleContext.cpp:597
(Diff revision 2)
>    }
>    if (aParentContext->ShouldSuppressLineBreak()) {
>      // Line break suppressing bit is propagated to any children of
>      // line participants, which include inline, contents, and inline
>      // ruby boxes.
> -    if (aParentDisplay->mDisplay == mozilla::StyleDisplay::Inline ||
> +    auto display = aParentDisplay->Display();

Nit: how about "parentDisplayVal" (so it doesn't look like the value of display from aContext, and also doesn't confusingly look similar to "aParentDisplay", which it would if just renamed to "parentDisplay").

::: layout/style/GeckoStyleContext.cpp:927
(Diff revision 2)
>        // If we're in this code, then mOriginalDisplay doesn't matter
>        // for purposes of the cascade (because this nsStyleDisplay
>        // isn't living in the ruletree anyway), and for determining
>        // hypothetical boxes it's better to have mOriginalDisplay
> -      // matching mDisplay here.
> +      // matching Display() here.

I think it's fine to continue to refer to mDisplay the field in the comment here.

::: layout/style/nsCSSPropList.h:481
(Diff revision 2)
> -    offsetof(nsStyleDisplay, mBackfaceVisibility),
> +    CSS_PROP_NO_OFFSET,
>      eStyleAnimType_Discrete)

Am I right that we never need to take the address of fields that are eStyleAnimType_Discrete or eStyleAnimType_None?  (I think that's the case but I'm not sure.)

::: layout/style/nsRuleNode.cpp:218
(Diff revision 2)
>  //    (so that we don't have to support a list-item root node), this method
>  //    lets the caller pick either behavior, using the 'aConvertListItem' arg.
>  //    Reference: http://www.w3.org/TR/CSS21/visuren.html#dis-pos-flo
>  /* static */
> -void
> -nsRuleNode::EnsureBlockDisplay(StyleDisplay& display,
> +StyleDisplay
> +nsRuleNode::EnsureBlockDisplay(StyleDisplay display,

Nit: While you're here, do you want to rename this to aDisplay?

::: layout/style/nsRuleNode.cpp:273
(Diff revision 2)
>  // EnsureInlineDisplay:
>  //  - if the display value (argument) is not an inline type
>  //    then we set it to a valid inline display value
>  /* static */
> -void
> -nsRuleNode::EnsureInlineDisplay(StyleDisplay& display)
> +StyleDisplay
> +nsRuleNode::EnsureInlineDisplay(StyleDisplay display)

Nit: and here?

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8903192 [details]
Bug 1395586: Other build fixes (to be squashed too).

https://reviewboard.mozilla.org/r/174980/#review180914

Looks like there's one instance of mAppearance in nsNativeThemeWin.cpp too?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903191 [details]
Bug 1395586: layout/style build fixes (to be squashed).

https://reviewboard.mozilla.org/r/174978/#review180900

> Am I right that we never need to take the address of fields that are eStyleAnimType_Discrete or eStyleAnimType_None?  (I think that's the case but I'm not sure.)

Yes, this is right. There's an assert to that effect.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

This is obsolete if we refactor the stuff to use the rust types, which is a big refactor, but we agreed it was worth it.