Bug 105065 – Free one bit in RenderObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

In order to implement bug 104829, I need to steal a bit on RenderObject as the class is completely packed.

[Simon Fraser (smfr)]

Get rid of the 'being dragged' bit.

[Simon Fraser (smfr)]

We we really want to waste a bit for this? I think a bit would be put to more use for something like bug 102617. Seems like it would be better to track things that need pre-layout via some side-band mechanism.

[Julien Chaffraix]

Created attachment 179555 [details]
Proposed stealing: Pack the positioned bits more efficiently to save one bit.

[Julien Chaffraix]

Simon, I didn't see your comments and already have made some efforts around free'ing one bit (that doesn't mean we *have* to use it for bug 104829).

> We we really want to waste a bit for this? I think a bit would be put to more use for something like bug 102617. Seems like it would be better to track things that need pre-layout via some side-band mechanism.

Not sure about the out-of-band mechanism, we are going with a tree traversal so that means we will have to do some hash lookup for each renderer which could end up being expensive.

> Get rid of the 'being dragged' bit.

AFAICT the 'being dragged bit' applies to a subtree so I am not 100% sure it can be removed easily. Another candidate was the reflection bit but we would need some data (it feels rare). I went for a free bit.

[Ojan Vafai]

Comment on attachment 179555 [details]
Proposed stealing: Pack the positioned bits more efficiently to save one bit.

View in context: https://bugs.webkit.org/attachment.cgi?id=179555&action=review

> Source/WebCore/rendering/RenderObject.h:1122
> +        void setPositionedState(int positionState) { m_positionedState = static_cast<PositionedState>(positionState & 0x3); }

This deserves a comment that we are doing this to save a bit by mapping AbsolutePosition and FixedPosition to the same state.

[Simon Fraser (smfr)]

Comment on attachment 179555 [details]
Proposed stealing: Pack the positioned bits more efficiently to save one bit.

View in context: https://bugs.webkit.org/attachment.cgi?id=179555&action=review

> Source/WebCore/rendering/style/RenderStyleConstants.h:102
> +    StaticPosition = 0,
> +    RelativePosition = 1,
> +    AbsolutePosition = 2,
> +    /* Required to pack our bits efficiently in RenderObject. */
> +    FixedPosition = 6,
> +    StickyPosition = 3

This ordering is confusing. Why not put StickyPosition in order? It would be clearer why you're using 6 for FixedPosition if you declared this with bits:

const unsigned InFlowPositionBit = 0
const unsigned OutOfFlowPositionBit = 1

FixedPosition = 1 << 2 | 1 <<  OutOfFlowPositionBit
or something.
We could also add isInFlowPositioned() const { return foo & (1 << InFlowPositionBit) } at some point.

[Julien Chaffraix]

Comment on attachment 179555 [details]
Proposed stealing: Pack the positioned bits more efficiently to save one bit.

View in context: https://bugs.webkit.org/attachment.cgi?id=179555&action=review

>> Source/WebCore/rendering/RenderObject.h:1122
>> +        void setPositionedState(int positionState) { m_positionedState = static_cast<PositionedState>(positionState & 0x3); }
> 
> This deserves a comment that we are doing this to save a bit by mapping AbsolutePosition and FixedPosition to the same state.

Sounds good.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:102
>> +    StickyPosition = 3
> 
> This ordering is confusing. Why not put StickyPosition in order? It would be clearer why you're using 6 for FixedPosition if you declared this with bits:
> 
> const unsigned InFlowPositionBit = 0
> const unsigned OutOfFlowPositionBit = 1
> 
> FixedPosition = 1 << 2 | 1 <<  OutOfFlowPositionBit
> or something.
> We could also add isInFlowPositioned() const { return foo & (1 << InFlowPositionBit) } at some point.

First, OK for the ordering.

It would be possible to do what you propose but there is a big downside. However screwy this code looks like, it makes it very efficient to convert from EPosition to PositionedState and what makes this possible is that AbsolutePosition and FixedPosition have the same lower bits but differ on the high bit, which we mask out.

With your scheme, I can't think of a way that wouldn't include a branch (there may be a way with some pretty crazy bit arithmetic).

[Simon Fraser (smfr)]

(In reply to comment #7)
> >> Source/WebCore/rendering/style/RenderStyleConstants.h:102
> >> +    StickyPosition = 3
> > 
> > This ordering is confusing. Why not put StickyPosition in order? It would be clearer why you're using 6 for FixedPosition if you declared this with bits:
> > 
> > const unsigned InFlowPositionBit = 0
> > const unsigned OutOfFlowPositionBit = 1
> > 
> > FixedPosition = 1 << 2 | 1 <<  OutOfFlowPositionBit
> > or something.
> > We could also add isInFlowPositioned() const { return foo & (1 << InFlowPositionBit) } at some point.
> 
> First, OK for the ordering.
> 
> It would be possible to do what you propose but there is a big downside. However screwy this code looks like, it makes it very efficient to convert from EPosition to PositionedState and what makes this possible is that AbsolutePosition and FixedPosition have the same lower bits but differ on the high bit, which we mask out.
> 
> With your scheme, I can't think of a way that wouldn't include a branch (there may be a way with some pretty crazy bit arithmetic).

 My scheme has the same bit layout as your scheme, unless I was misunderstanding. I was just asking for your scheme's bit layout to be more explicit in the code.

[Julien Chaffraix]

> > With your scheme, I can't think of a way that wouldn't include a branch (there may be a way with some pretty crazy bit arithmetic).
> 
>  My scheme has the same bit layout as your scheme, unless I was misunderstanding. I was just asking for your scheme's bit layout to be more explicit in the code.

Now you got me confused, not about the goal but the proposal in comment #6: 

const unsigned OutOfFlowPositionBit = 1

FixedPosition = 1 << 2 | 1 <<  OutOfFlowPositionBit

As defined the bit is set on both AbsolutePosition and FixedPosition but also on StickyPosition (= 3), which makes the name confusing. If you change StickPosition value to hold having an OutOfFlowPositionBit is where it adds more conversion complexity.

[Simon Fraser (smfr)]

I got my bits wrong, sorry.

[Julien Chaffraix]

Thanks Simon, I wanted to make sure I wasn't missing something obvious. Re-titling the bug as we will use an out-of-band mechanism for bug 104829 and see how this goes. We can always change our mind later if we find some performance bottleneck.

[Julien Chaffraix]

Created attachment 179962 [details]
Updated fix: added the comment ojan's requested.

[Julien Chaffraix]

Comment on attachment 179962 [details]
Updated fix: added the comment ojan's requested.

View in context: https://bugs.webkit.org/attachment.cgi?id=179962&action=review

> Source/WebCore/rendering/style/RenderStyleConstants.h:101
> +    /* This value is required to pack our bits efficiently in RenderObject. */

This C style comment slipped in, I will fix before landing.

[Julien Chaffraix]

Created attachment 180051 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180051 [details]
Patch for landing

Clearing flags on attachment: 180051

Committed r138113: <http://trac.webkit.org/changeset/138113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.