|
|
DescriptionImproved bitfield packing behavior in ComputedStyleBase
Improved bitfield packing behavior in ComputedStyleBase to greedily
group fields into buckets of 32 bits, and place those fields adjacently.
This prevents against padding bits added from the naive implementation
of just sorting the fields in decreasing order of size.
Also added an ASSERT_SIZE to ComputedStyleBase to ensure this packing
has produced the optimal solution.
BUG=628043
Committed: https://crrev.com/b319a2f99e630380875c015a50fd20d5dd41ac3a
Cr-Commit-Position: refs/heads/master@{#441594}
Patch Set 1 #Patch Set 2 : Fixed optimal size calculation #Patch Set 3 : Small wording fix #
Total comments: 1
Patch Set 4 : Updated comments to make it clearer what to do when asserts fail #Patch Set 5 : Introduced 'extra_padding_bytes' variable to explicitly show additional padding when added #Patch Set 6 : Removed 32 bit reference #
Total comments: 1
Messages
Total messages: 28 (15 generated)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sashab@chromium.org changed reviewers: + aazzam@google.com, loyso@chromium.org, meade@chromium.org
Thoughts welcome! :)
lgtm nice! Good job making a reasonably short solution :) https://codereview.chromium.org/2611633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2611633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:147: # crosses the 32 bit boundary line. To do this, group fields into buckets I think this comment is mildly misleading - maybe something more like "Since fields cannot cross 32 bit word boundaries, in order to minimize padding, group fields into buckets so that as many buckets as possible are exactly 32 bits. ..."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks eddy - done :) Alexey - ptal at comment & new extra_padding_bytes variable to explicitly show overhead from non-optimal packing structure :)
On 2017/01/05 at 00:05:19, sashab wrote: > Thanks eddy - done :) > > Alexey - ptal at comment & new extra_padding_bytes variable to explicitly show overhead from non-optimal packing structure :) extra_padding_bytes approach LGTM. nit: "32 bit word" how about ARM64 and x86-64? They are our target platforms as well.
lgtm
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Removed reference to 32 bits. Thanks all for taking a look! :)
The CQ bit was checked by sashab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, loyso@chromium.org, aazzam@google.com Link to the patchset: https://codereview.chromium.org/2611633003/#ps100001 (title: "Removed 32 bit reference")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/05 00:36:52, loyso wrote: > On 2017/01/05 at 00:05:19, sashab wrote: > > Thanks eddy - done :) > > > > Alexey - ptal at comment & new extra_padding_bytes variable to explicitly show > overhead from non-optimal packing structure :) > > extra_padding_bytes approach LGTM. > > nit: "32 bit word" how about ARM64 and x86-64? They are our target platforms as > well. Is there a way to find out the target architecture while compiling? We could add a TODO to generate differently depending on that. Otherwise, I still think it's worth mentioning why that 32 is there, plus a comment that it should pack reasonably well for 64 bit architectures if it packs well for 32 bit ones, although it might not be quite optimal.
https://codereview.chromium.org/2611633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl (right): https://codereview.chromium.org/2611633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl:15: ASSERT_SIZE(ComputedStyleBase, SameSizeAsComputedStyleBase); Out of curiosity.. Can we use just static_assert(sizeof(ComputedStyleBase), {{expected_total_field_bytes}}UL); here?
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483589896202750, "parent_rev": "cc8744429ebd3fc507c0ed9895bcf857b52d6f05", "commit_rev": "cc53ddbb49d378d18003915a0f2cded1a4dfea31"}
Message was sent while issue was closed.
Description was changed from ========== Improved bitfield packing behavior in ComputedStyleBase Improved bitfield packing behavior in ComputedStyleBase to greedily group fields into buckets of 32 bits, and place those fields adjacently. This prevents against padding bits added from the naive implementation of just sorting the fields in decreasing order of size. Also added an ASSERT_SIZE to ComputedStyleBase to ensure this packing has produced the optimal solution. BUG=628043 ========== to ========== Improved bitfield packing behavior in ComputedStyleBase Improved bitfield packing behavior in ComputedStyleBase to greedily group fields into buckets of 32 bits, and place those fields adjacently. This prevents against padding bits added from the naive implementation of just sorting the fields in decreasing order of size. Also added an ASSERT_SIZE to ComputedStyleBase to ensure this packing has produced the optimal solution. BUG=628043 Review-Url: https://codereview.chromium.org/2611633003 Committed: https://chromium.googlesource.com/chromium/src/+/cc53ddbb49d378d18003915a0f2c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/cc53ddbb49d378d18003915a0f2c...
Message was sent while issue was closed.
Description was changed from ========== Improved bitfield packing behavior in ComputedStyleBase Improved bitfield packing behavior in ComputedStyleBase to greedily group fields into buckets of 32 bits, and place those fields adjacently. This prevents against padding bits added from the naive implementation of just sorting the fields in decreasing order of size. Also added an ASSERT_SIZE to ComputedStyleBase to ensure this packing has produced the optimal solution. BUG=628043 Review-Url: https://codereview.chromium.org/2611633003 Committed: https://chromium.googlesource.com/chromium/src/+/cc53ddbb49d378d18003915a0f2c... ========== to ========== Improved bitfield packing behavior in ComputedStyleBase Improved bitfield packing behavior in ComputedStyleBase to greedily group fields into buckets of 32 bits, and place those fields adjacently. This prevents against padding bits added from the naive implementation of just sorting the fields in decreasing order of size. Also added an ASSERT_SIZE to ComputedStyleBase to ensure this packing has produced the optimal solution. BUG=628043 Committed: https://crrev.com/b319a2f99e630380875c015a50fd20d5dd41ac3a Cr-Commit-Position: refs/heads/master@{#441594} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b319a2f99e630380875c015a50fd20d5dd41ac3a Cr-Commit-Position: refs/heads/master@{#441594}
Message was sent while issue was closed.
Re meade - I'd rather not complicate the code with heaps of #ifdefs to figure out the target padding size. Word size is not really enough because even on 64 bit platforms you have 32 bit ints, so some platforms must still pack two 32 bit ints into one 64 bit word. It would have to be some custom metric that I don't think is necessary. Re loyso - different platforms add different overhead for classes, which is why the ASSERT_SIZE macro was introduced. If it fails, it prints the expected vs actual size, so you don't lose any information here. I'd be wary about hard-coding actual byte numbers in the code for fear it would be different on different platforms -- see ASSERT_SIZE in ComputedStyle.cpp.
Message was sent while issue was closed.
sashab@chromium.org changed reviewers: + dcheng@chromium.org, esprehn@chromium.org, napper@chromium.org
Message was sent while issue was closed.
+for fyi esprehn, napper, dcheng Pretty cool huh? :D Part of me wants to do this automatically as a clang plugin for every class in Chrome... But, that's a ways off for now. :) |