Bug 64914 - REGRESSION (r82611) InlineBox has 33 bits of bitset, causing alignment issues and extra memory use
Summary: REGRESSION (r82611) InlineBox has 33 bits of bitset, causing alignment issues...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
: 65695 (view as bug list)
Depends on: 69168 69170
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-20 18:00 PDT by WebKit Review Bot
Modified: 2011-10-15 01:54 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (3.07 KB, patch)
2011-08-27 05:47 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for landing (r=anttik) (3.66 KB, patch)
2011-09-30 12:33 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Speculative Win32 tweak for EWS. (3.68 KB, patch)
2011-10-03 03:43 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
EWS Y U NO process this patch? (3.68 KB, patch)
2011-10-03 08:35 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2011-07-20 18:00:06 PDT
REGRESSION(82611) InlineBox has 33 bits of bitset, causing alignment issues and extra memory use
Requested by jamesr on #webkit.
Comment 1 Simon Fraser (smfr) 2011-07-20 18:04:09 PDT
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.
Comment 2 James Robinson 2011-07-20 18:05:11 PDT
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.
Comment 3 Simon Fraser (smfr) 2011-08-04 10:25:15 PDT
*** Bug 65695 has been marked as a duplicate of this bug. ***
Comment 4 Andreas Kling 2011-08-27 05:25:42 PDT
(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.
Comment 5 Andreas Kling 2011-08-27 05:47:04 PDT
Created attachment 105434 [details]
Proposed patch
Comment 6 Benjamin Poulain 2011-08-27 06:00:47 PDT
Comment on attachment 105434 [details]
Proposed patch

Gosh, for an unused function, that is ridiculous.
Comment 7 Andreas Kling 2011-08-27 06:23:02 PDT
(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, ...)
Comment 8 Andreas Kling 2011-09-30 12:33:17 PDT
Created attachment 109325 [details]
Patch for landing (r=anttik)
Comment 9 WebKit Review Bot 2011-09-30 13:48:32 PDT
Comment on attachment 109325 [details]
Patch for landing (r=anttik)

Clearing flags on attachment: 109325

Committed r96422: <http://trac.webkit.org/changeset/96422>
Comment 10 WebKit Review Bot 2011-09-30 13:48:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Andreas Kling 2011-10-03 03:43:28 PDT
Created attachment 109461 [details]
Speculative Win32 tweak for EWS.
Comment 12 Andreas Kling 2011-10-03 08:35:19 PDT
Created attachment 109488 [details]
EWS Y U NO process this patch?

Trying EWS again.. Also reopening bug since the patch was rolled out.
Comment 13 Andreas Kling 2011-10-05 04:40:32 PDT
Relanded in <http://trac.webkit.org/changeset/96693>.
Comment 14 Adam Barth 2011-10-15 01:51:59 PDT
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.