Bug 64914 – REGRESSION (r82611) InlineBox has 33 bits of bitset, causing alignment issues and extra memory use

• Component: New Bugs
• Status: RESOLVED FIXED

[WebKit Review Bot]

REGRESSION(82611) InlineBox has 33 bits of bitset, causing alignment issues and extra memory use
Requested by jamesr on #webkit.

[Simon Fraser (smfr)]

Dan says that bidiEmbeddingLevel has to be 6 bits, to comply with the Unicode standard.
m_expansion is a count of pixels used for justification.

Any fix for this bug should add compile-time asserts.

[James Robinson]

http://trac.webkit.org/changeset/82611 added a bit to all InlineBoxes to optimize some overflow cases, but that also grew the number of bits stored at the end of InlineBox from 32 to 33.  Due to alignment this costs extra memory, possibly an extra word, and probably also makes accessing m_expansion slower since it now occupies parts of 3 bytes instead of 2.  This was a big speedup, so we should look for something else to save the space.

According to https://bugs.webkit.org/show_bug.cgi?id=57178 we can delete 4 unused bits.  That patch was rolled out due to some GTK accessibility crap depending on it, but that might be easy to fix.

Does m_expansion have to be 11 bits?  There might be other bits in here that aren't super helpful.

It would be great to have compile time assertions on object size to avoid regressing the size of core objects like this in the future.

[Simon Fraser (smfr)]

*** Bug 65695 has been marked as a duplicate of this bug. ***

[Andreas Kling]

(In reply to comment #2)
> Does m_expansion have to be 11 bits?

It used to be 12 bits, back when it was called m_toAdd. Then it was silently reduced to 11 bits in bug 46386.

It's fairly trivial to create a document that will overflow this bitfield, whether it's 10 or 11 bits wide. Someone who knows more about text layout should comment though.

[Andreas Kling]

Created attachment 105434 [details]
Proposed patch

[Benjamin Poulain]

Comment on attachment 105434 [details]
Proposed patch

Gosh, for an unused function, that is ridiculous.

[Andreas Kling]

(In reply to comment #1)
> Any fix for this bug should add compile-time asserts.

What would be the best way of doing that?

I could add something like:
COMPILE_ASSERT(sizeof(InlineBox) == 56 || sizeof(InlineBox) == 32, SizeOfInlineBox);

But that could break depending on compiler flags (struct packing, alignment, ...)

[Andreas Kling]

Created attachment 109325 [details]
Patch for landing (r=anttik)

[WebKit Review Bot]

Comment on attachment 109325 [details]
Patch for landing (r=anttik)

Clearing flags on attachment: 109325

Committed r96422: <http://trac.webkit.org/changeset/96422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

Created attachment 109461 [details]
Speculative Win32 tweak for EWS.

[Andreas Kling]

Created attachment 109488 [details]
EWS Y U NO process this patch?

Trying EWS again.. Also reopening bug since the patch was rolled out.

[Andreas Kling]

Relanded in <http://trac.webkit.org/changeset/96693>.

[Adam Barth]

Comment on attachment 105434 [details]
Proposed patch

Cleared Benjamin Poulain's review+ from obsolete attachment 105434 [details] so that this bug does not appear in http://webkit.org/pending-commit.