Last Comment Bug 717311 - gfxUnicodePropertyData.cpp:sCatEAWValues takes up more space than it should
: gfxUnicodePropertyData.cpp:sCatEAWValues takes up more space than it should
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nathan Froyd [:froydnj]
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2012-01-11 10:57 PST by Nathan Froyd [:froydnj]
Modified: 2012-01-13 01:06 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

pack bits manually (459.93 KB, patch)
2012-01-11 11:04 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
change bitfield datatype (2.94 KB, patch)
2012-01-11 16:34 PST, Nathan Froyd [:froydnj]
jfkthame: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-01-11 10:57:28 PST
gfxUnicodePropertyData.cpp:sCatEAWValues is an array of:

struct {
  unsigned int mEAW:3;
  unsigned int mCategory:5;

Unfortunately, this actually winds up wasting space because of structure padding inserted to maintain alignment.

Patch coming up.
Comment 1 Nathan Froyd [:froydnj] 2012-01-11 11:04:11 PST
Created attachment 587753 [details] [diff] [review]
pack bits manually

Instead of using a structure with bitfields, pack things manually.

Saves a little over 70K of data.
Comment 2 Jonathan Kew (:jfkthame) 2012-01-11 13:38:59 PST
Hmm... you're right, it does end up wasting space. :( However, wouldn't a simpler patch be to declare the fields as unsigned char (with the appropriate field widths)? That would avoid the need for manually packing/unpacking the fields, which just makes the code more opaque.
Comment 3 Nathan Froyd [:froydnj] 2012-01-11 16:34:04 PST
Created attachment 587875 [details] [diff] [review]
change bitfield datatype

Indeed, that works just as well and produces a much smaller patch. :)
Comment 4 Jonathan Kew (:jfkthame) 2012-01-12 00:35:08 PST
Comment on attachment 587875 [details] [diff] [review]
change bitfield datatype

Great, let's go with this - thanks.
Comment 6 Ed Morley [:emorley] 2012-01-12 05:38:19 PST
Backed out of inbound, since that push caused test crashes, though presumably just bug 708075:
Comment 7 Jonathan Kew (:jfkthame) 2012-01-12 07:58:46 PST
Re-landed on inbound, as this wasn't responsible for the problems on the previous push:
Comment 8 Marco Bonardo [::mak] 2012-01-13 01:06:19 PST

Note You need to log in before you can comment on or make changes to this bug.