1257732 - Need more frame state bits in nsFrameStateBits.h

Status: NEW

(Core :: Layout, defect)

Description

[Jet Villegas (inactive)]

All 64 bits of nsFrame and nsBlockFrame state are used up. 

More info on how to fix this are posted here:
https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/jUAvkOzuoMI

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Bits 46 and 56 seem to be available still, in nsFrameStateBits.h

Comment 2

[Kartikaya Gupta (email:[e-mail])]

And bit 55.

Comment 3

[Jet Villegas (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Bits 46 and 56 seem to be available still, in nsFrameStateBits.h

Good to know! That buys us more time, though we expect to use up more bits as part of DisplayList optimization.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip1, convert nsTextFrame bits

I agree with Xidorn's proposal on m.d.t.layout that we should move class-specific
bits to their respective class.  That makes it impossible to set those bits
on frames they weren't meant for (we have had such bugs in the past).

It should also save space compared to reserving more than 64 bits for all
frame types.  Most frame types don't need any, or very few, specific bits.
There are a few that use a lot of bits though, like nsTextFrame, XUL frames,
nsBlockFrame.

I took a stab at implementing this to see what it looks like.
I chose to convert nsTextFrame which is the worst (most bits).
Based on this patch it seems like the way to go.  Other classes are likely
easier to convert.

Thoughts?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

I'd also suggest we revisit existing bits, and try to release some of the them via converting them to a virtual call, e.g. probably NS_FRAME_IS_NONDISPLAY, which is only bound to specific frame classes. We can probably also try to move some of the bits into nsStyleContext, where there are more slots available, e.g. NS_FRAME_GENERATED_CONTENT.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I guess nsTextFrame is the most important one we want to avoid bloating...

Comment 7

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #6)
> I guess nsTextFrame is the most important one we want to avoid bloating...

With the disclaimer that haven't checked: at the moment this change shouldn't
change the size.  This is because nsTextFrame has an odd number of 32-bit fields
at the end, and the added bits are less than 32 bits, so they just use a gap that's
already there.  (nsTextFrame is allocated from the shell arena which always rounds
up the size to the nearest 64-bit word.)  Granted, once we need another 32-bit
field on nsTextFrame for other reasons these bits will cost a 64-bit word.
But the risk of boundary effects like that is generally unavoidable, and I don't
think moving bits to sub-classes makes it worse.

Actually, moving to bool:1 members everywhere would allow us to optimize memory
size by putting them in a base class that happens to have alignment spill.
There would still be a small risk that of using say mIsOnlyWhitespace on a non-
text frame, but that risk is very small compared to the current risk of setting
some class-specific bit for that frame by collision.  It'd cost engineering time
to maintain such a scheme though - we'd need to track frame sizes and move bits
around if needed...  Not sure if that's really worth it in general, but it might
be for nsTextFrame perhaps.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think we're going to want a decent number of new bits for things we discover in quantum flow.

I was thinking we should just bite the bullet and add another 64 bits worth, and do this by adding a block of "unsigned mFoo : 1" member variables (unless we've learned that it now works with bool rather than unsigned) -- hopefully in a place where it will only take up 32 bits of space on 32-bit builds.

However, I think we actually have 24 bits free after mWritingMode in both 32-bit and 64-bit builds, for most frame classes, unless I'm misreading, so that seems like a good place to start.

I think we should start adding mFoo bits right after mWritingMode, with a comment that we have 24 bits of space there right now.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #8)
> I think we should start adding mFoo bits right after mWritingMode, with a
> comment that we have 24 bits of space there right now.

I think jfkthame wants to use these 3 bytes for bidi data in bug 1348469 comment 10. Not sure which one is more worthy.

Comment 10

[Mats Palmgren (inactive)]

Another option would be use the 64 bits we have more efficiently.
Very few of the reserved frame-specific bits are actually needed by
most frame types - only Box and Block seems to use more than a few.
So we could convert some of those bits to nsBoxFrame / nsBlockFrame
bool:1 members and then reclaim them as Generic bits.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should be using a new bit on average every day for at least a few weeks if we're fixing quantum flow bugs at appropriate rate, so I think we should just use up another word of bits, then.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #9)
> (In reply to David Baron :dbaron: ⌚️UTC+8 from comment #8)
> > I think we should start adding mFoo bits right after mWritingMode, with a
> > comment that we have 24 bits of space there right now.
> 
> I think jfkthame wants to use these 3 bytes for bidi data in bug 1348469
> comment 10. Not sure which one is more worthy.

Yes, I filed bug 1354218 with a patch for this (awaiting review).

Comment 13

[Mats Palmgren (inactive)]

BTW, Matt Woodrow had some ideas for reclaiming some of the paint related bits
in bug 1342009 comment 25.