Closed Bug 1294628 Opened 8 years ago Closed 8 years ago

Replace int16_t BlockReflowInput::mFlag with bit fields struct

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(1 file)

Bug 1294628 - Replace block reflow input flags with a bit fields struct.
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
It's good to make block reflow input flags to be wrapped into a bit fields struct like ReflowInputFlags [1] such that
1) The camel case flags name should be easier to read, and
2) the type safety is guaranteed. (One might pass wrong flag name into GetFlag or SetFlag.)

[1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/layout/generic/ReflowInput.h#178
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68750

::: layout/generic/BlockReflowInput.h:34
(Diff revision 1)
> +
> +    // Set in the BlockReflowInput constructor when the frame being reflowed has
> +    // been given NS_UNCONSTRAINEDSIZE as its available BSize in the
> +    // ReflowInput. If set, NS_UNCONSTRAINEDSIZE is passed to nsLineLayout as
> +    // the available BSize.
> +    uint16_t mHasUnconstrainedBSize: 1;

Two initial high-level nits:
 (1) I don't think we should use uint16_t here.  It looks like these flag are all meant to be interpreted as booleans, so as long as we're modernizing their representation here, we should just go straight to "bool mFoo:1".

(I suspect you're using "uint16_t" for consistency with ReflowInput's flags.  But it looks to me like those flags' uint16_t-bitfield-type is simply an artifact of history, and is not anything particularly important/useful.  In particular: that 16-bit type seems to date back to e.g. this old version of the code:
http://searchfox.org/mozilla-central/rev/d63551e2ffbdec354c5d12b83d2595ce23365f0d/layout/generic/nsHTMLReflowState.h#246
At that point in time, it seems we had an intended-to-be-16-bits-IN-TOTAL bitfield, with a few PRUint16 mFoo:1 variables followed by PRUint16 mUnused:12 to explicitly eat up the rest.  I suspect that's the only reason we happen to be left with uint16_t there now.  So, bool mFoo:1 seems strictly better to me, from a semantic & safety perspective -- i.e. it should make it harder to accidentally do the wrong sorts of operations with these flags.)

(2) Nit: please drop the space between the ":" and the "1" in each of these variable declarations, for consistency with ReflowInput's bitfield (and with the prevailing style in Gecko).  Use "mFoo:1", not "mFoo: 1".

(I only find a few examples of Gecko code that uses this "mFoo: 1" style, vs. manymany examples that use "mFoo:1". Here are the commands I used to search, FWIW:
* With space:
grep -r "m[A-Z].*[^ ]: 1.*;" * | grep "\.h:" | wc -l
* Without space:
grep -r "m[A-Z].*[^ ]:1.*;" * | grep "\.h:" | wc -l
)
(One concrete reason to prefer bool over uint16_t for these bitfields -- based on local testing, an expression like "mHasUnconstrainedBSize++" will compile just fine (which is bad!!) if it has type uint16_t:1, but not if it has type bool:1.  clang issues a -Wdeprecated-increment-bool warning if the type is a bool, which will be promoted to a build error on our build boxes with fatal warnings.)
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68752

r- for now, but I'll grant r+ with previous comment & the following addressed.

::: layout/generic/BlockReflowInput.h:78
(Diff revision 1)
> -#define BRS_HAVELINEADJACENTTOTOP 0x00000020
> -// Set when the block has the equivalent of NS_BLOCK_FLOAT_MGR
> -#define BRS_FLOAT_MGR             0x00000040
> +
> +    // Set when mLineAdjacentToTop is valid.
> +    uint16_t mHasLineAdjacentToTop: 1;
> +
> +    // Set when the block has the equivalent of NS_BLOCK_FLOAT_MGR.
> +    uint16_t mHasFloatManager: 1;

tl;dr: please rename this to mBlockNeedsFloatManager.

Longer discussion: So, this patch is introducing "Has" into this flag's name. (It was formerly named BRS_FLOAT_MGR, without the word "Has".) Unfortunately, "Has" gives the wrong impression here.

In particular -- this object happens to contain a member-var called "mFloatManager" -- so a flag called "mHasFloatManager" sounds a lot like it would just indicate whether mFloatManager happens to be non-null.  BUT, it does not actually indicate that at all -- it looks like mFloatManager is expected to *always* be non-null, regardless of this particular flag's value.  So, mHasFloatManager would be a very confusing name here.

That this bit gets *set* in a clause that uses "Needs" language, not "Has" language:
  if (aBlockNeedsFloatManager) {
    SetFlag(BRS_FLOAT_MGR, true);

So, this should probably be renamed to something "needs"-flavored -- perhaps even just "mBlockNeedsFloatManager" to exactly match that argument's name?  That'll be nice, so that in the future, if we come up with a better name, we'll know to adjust *both* the flag and the identically-named argument at the same time.

(I think the concept it represents is basically "do we need our *own* float manager" -- that might really be ultimately what we want to express in this variables's name. I'm not sure, though -- so for the time being, best to just match the language used in the existing aBlockNeedsFloatManager parameter, I think.)

::: layout/generic/BlockReflowInput.h:86
(Diff revision 1)
> -#define BRS_FLOAT_FRAGMENTS_INSIDE_COLUMN_ENABLED 0x00000400
> -#define BRS_LASTFLAG              BRS_FLOAT_FRAGMENTS_INSIDE_COLUMN_ENABLED
>  
> -namespace mozilla {
> +    uint16_t mIsOverflowContainer: 1;
>  
> -// BlockReflowInput contains additional reflow state information that the
> +    // Our mPushedFloats list is stored on the block's property table.

s/Our/Set when our/  (for consistency with how the other flags here are documented.)

::: layout/generic/BlockReflowInput.cpp:58
(Diff revision 1)
>                                   "layout.float-fragments-inside-column.enabled");
>    }
> -  SetFlag(BRS_FLOAT_FRAGMENTS_INSIDE_COLUMN_ENABLED, sFloatFragmentsInsideColumnEnabled);
> -  
> +  mFlags.mFloatFragmentsInsideColumnEnabled = sFloatFragmentsInsideColumnEnabled;
> +
>    WritingMode wm = aReflowInput.GetWritingMode();
> -  SetFlag(BRS_ISFIRSTINFLOW, aFrame->GetPrevInFlow() == nullptr);
> +  mFlags.mIsFirstInflow = aFrame->GetPrevInFlow() == nullptr;

Two gripes about this line:
 (1) It's ending up in the form a = b == c which is hard to read.
 (2) The explicit nullptr-check here (which predates your patch) goes against a gecko coding style guideline.

Happily, we can address both gripes with a single simplification -- just replace the null comparison with "!", like so:
  mFlags.mIsFirstInflow  = !aFrame->GetPrevInFlow();

::: layout/generic/BlockReflowInput.cpp:136
(Diff revision 1)
>      mContentArea.BSize(wm) = std::max(0, mBEndEdge - mBorderPadding.BStart(wm));
>    }
>    else {
>      // When we are not in a paginated situation then we always use
>      // an constrained height.
> -    SetFlag(BRS_UNCONSTRAINEDBSIZE, true);
> +    mFlags.mHasUnconstrainedBSize = true;

While you're here, please also fix the comment right above this line:
 s/an constrained/a constrained/

("an" is incorrect there)
Attachment #8780413 - Flags: review?(dholbert) → review-
Also: before landing, please sanity-check that this isn't somehow increasing the size of this BlockReflowInput struct on our 3 main desktop platforms.  (For this sort of change, that's worth worrying about -- since bitfield-packing can be weird, particularly across compilers, and we don't want to regress the size of this object without noticing.)

So -- I'd suggest you add a new local patch, which introduces an easily-searchable printf() to e.g. the BlockReflowInput constructor, something like
  printf("***********BlockReflowInput size: %d\n", sizeof(*this));
...and then push that patch to try on its own, and then again with this bug's patch (so 2 total try runs with that patch), and compare the values that it prints out.  A reftest-only opt Try run should be sufficient for that, I think.

Probably worth testing that on Windows, Mac, & Linux, with opt builds [i.e. the configuration that our users run].  Worth testing each of those platforms in case there are any compiler/platform-specific quirks.
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #2)
> (2) Nit: please drop the space between the ":" and the "1" in each of these
> variable declarations, for consistency with ReflowInput's bitfield (and with
> the prevailing style in Gecko).  Use "mFoo:1", not "mFoo: 1".
> 
> (I only find a few examples of Gecko code that uses this "mFoo: 1" style,
> vs. manymany examples that use "mFoo:1". Here are the commands I used to
> search, FWIW:
> * With space:
> grep -r "m[A-Z].*[^ ]: 1.*;" * | grep "\.h:" | wc -l
> * Without space:
> grep -r "m[A-Z].*[^ ]:1.*;" * | grep "\.h:" | wc -l
> )

I myself usually use "bool mFoo : 1" (whitespaces on both sides of ":") which seems to be a more usual pattern.
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68978

::: layout/generic/BlockReflowInput.h:28
(Diff revision 1)
> -// BSize in the ReflowInput. If set, NS_UNCONSTRAINEDSIZE is passed to
> -// nsLineLayout as the available BSize.
> -#define BRS_UNCONSTRAINEDBSIZE    0x00000001
> -// BRS_ISBSTARTMARGINROOT is set in the BlockReflowInput constructor when
> -// reflowing a "block margin root" frame (i.e. a frame with the
> -// NS_BLOCK_MARGIN_ROOT flag set, for which margins apply by default).
> +// is read-only data that is passed down from a parent frame to its children.
> +class BlockReflowInput {
> +
> +  // Block reflow input flags.
> +  struct Flags {
> +    Flags() { memset(this, 0, sizeof(*this)); }

TBH, I don't really like using memset here. I'd prefer listing every fields here, which could (imaginarily) be more friendly for compilers and static analyser.

I really hope C++ has member initializers for bit fields...
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68750

> Two initial high-level nits:
>  (1) I don't think we should use uint16_t here.  It looks like these flag are all meant to be interpreted as booleans, so as long as we're modernizing their representation here, we should just go straight to "bool mFoo:1".
> 
> (I suspect you're using "uint16_t" for consistency with ReflowInput's flags.  But it looks to me like those flags' uint16_t-bitfield-type is simply an artifact of history, and is not anything particularly important/useful.  In particular: that 16-bit type seems to date back to e.g. this old version of the code:
> http://searchfox.org/mozilla-central/rev/d63551e2ffbdec354c5d12b83d2595ce23365f0d/layout/generic/nsHTMLReflowState.h#246
> At that point in time, it seems we had an intended-to-be-16-bits-IN-TOTAL bitfield, with a few PRUint16 mFoo:1 variables followed by PRUint16 mUnused:12 to explicitly eat up the rest.  I suspect that's the only reason we happen to be left with uint16_t there now.  So, bool mFoo:1 seems strictly better to me, from a semantic & safety perspective -- i.e. it should make it harder to accidentally do the wrong sorts of operations with these flags.)
> 
> (2) Nit: please drop the space between the ":" and the "1" in each of these variable declarations, for consistency with ReflowInput's bitfield (and with the prevailing style in Gecko).  Use "mFoo:1", not "mFoo: 1".
> 
> (I only find a few examples of Gecko code that uses this "mFoo: 1" style, vs. manymany examples that use "mFoo:1". Here are the commands I used to search, FWIW:
> * With space:
> grep -r "m[A-Z].*[^ ]: 1.*;" * | grep "\.h:" | wc -l
> * Without space:
> grep -r "m[A-Z].*[^ ]:1.*;" * | grep "\.h:" | wc -l
> )

"mFoo : 1" is most popular style as Xidorn said in comment 6. Here's the search result in gecko.

$ ag --cc "m[A-Z].*[^ ] : 1.*;" | wc -l
     559
$ ag --cc "m[A-Z].*[^ ]:1.*;" | wc -l
     142
$ ag --cc "m[A-Z].*[^ ]: 1.*;" | wc -l
      16

I'll follow the majority :)
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68752

> tl;dr: please rename this to mBlockNeedsFloatManager.
> 
> Longer discussion: So, this patch is introducing "Has" into this flag's name. (It was formerly named BRS_FLOAT_MGR, without the word "Has".) Unfortunately, "Has" gives the wrong impression here.
> 
> In particular -- this object happens to contain a member-var called "mFloatManager" -- so a flag called "mHasFloatManager" sounds a lot like it would just indicate whether mFloatManager happens to be non-null.  BUT, it does not actually indicate that at all -- it looks like mFloatManager is expected to *always* be non-null, regardless of this particular flag's value.  So, mHasFloatManager would be a very confusing name here.
> 
> That this bit gets *set* in a clause that uses "Needs" language, not "Has" language:
>   if (aBlockNeedsFloatManager) {
>     SetFlag(BRS_FLOAT_MGR, true);
> 
> So, this should probably be renamed to something "needs"-flavored -- perhaps even just "mBlockNeedsFloatManager" to exactly match that argument's name?  That'll be nice, so that in the future, if we come up with a better name, we'll know to adjust *both* the flag and the identically-named argument at the same time.
> 
> (I think the concept it represents is basically "do we need our *own* float manager" -- that might really be ultimately what we want to express in this variables's name. I'm not sure, though -- so for the time being, best to just match the language used in the existing aBlockNeedsFloatManager parameter, I think.)

Agree. Will change to mBlockNeedsFloatManager.

> Two gripes about this line:
>  (1) It's ending up in the form a = b == c which is hard to read.
>  (2) The explicit nullptr-check here (which predates your patch) goes against a gecko coding style guideline.
> 
> Happily, we can address both gripes with a single simplification -- just replace the null comparison with "!", like so:
>   mFlags.mIsFirstInflow  = !aFrame->GetPrevInFlow();

True. Quote from the style guideline: 
> When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review68978

> TBH, I don't really like using memset here. I'd prefer listing every fields here, which could (imaginarily) be more friendly for compilers and static analyser.
> 
> I really hope C++ has member initializers for bit fields...

OK. Will fix in next patch.
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #5)
> Also: before landing, please sanity-check that this isn't somehow increasing
> the size of this BlockReflowInput struct on our 3 main desktop platforms. 
> (For this sort of change, that's worth worrying about -- since
> bitfield-packing can be weird, particularly across compilers, and we don't
> want to regress the size of this object without noticing.)
> 
> So -- I'd suggest you add a new local patch, which introduces an
> easily-searchable printf() to e.g. the BlockReflowInput constructor,
> something like
>   printf("***********BlockReflowInput size: %d\n", sizeof(*this));
> ...and then push that patch to try on its own, and then again with this
> bug's patch (so 2 total try runs with that patch), and compare the values
> that it prints out.  A reftest-only opt Try run should be sufficient for
> that, I think.
> 
> Probably worth testing that on Windows, Mac, & Linux, with opt builds [i.e.
> the configuration that our users run].  Worth testing each of those
> platforms in case there are any compiler/platform-specific quirks.

Try run that prints BlockReflowInput and mFlags size (Search for log like "!!!!!! BlockReflowInput size 196, mFlags size 2")

Without my patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8df713298771&selectedJob=25717815

With my patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bfbb261b0f&selectedJob=25721148

On Windows, Mac, and Linux with opt builds, both BlockReflowInput and mFlags size remain the same after applying my patch. I don't get the result on Android opt build. Perhaps we need other method to print log on Android.
Comment on attachment 8780413 [details]
Bug 1294628 - Replace block reflow input flags with a bit fields struct.

https://reviewboard.mozilla.org/r/71140/#review69154

Thanks! r=me

(RE logging / sizeof() stuff -- indeed, Android logging is a bit trickier -- I believe stdout ends up in a separate log file there.  Anyway, I wouldn't bother spending extra time trying to check there - if the size is staying the same on Win/Mac/Linux, it almost certainly is on Android as well. [Plus, this struct only exists for the duration of a reflow anyway, so its size doesn't matter *too* much, if it did grow a little for arcane reasons.])
Attachment #8780413 - Flags: review?(dholbert) → review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8146a675a477
Replace block reflow input flags with a bit fields struct. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/8146a675a477
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
See Also: → 1312295
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: