In order to implement bug 104829, I need to steal a bit on RenderObject as the class is completely packed.
Get rid of the 'being dragged' bit.
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.
Created attachment 179555 [details] Proposed stealing: Pack the positioned bits more efficiently to save one bit.
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.
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.
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.
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).
(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.
> > 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.
I got my bits wrong, sorry.
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.
Created attachment 179962 [details] Updated fix: added the comment ojan's requested.
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.
Created attachment 180051 [details] Patch for landing
Comment on attachment 180051 [details] Patch for landing Clearing flags on attachment: 180051 Committed r138113: <http://trac.webkit.org/changeset/138113>
All reviewed patches have been landed. Closing bug.