Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(148)

Issue 2611633003: Improved bitfield packing behavior in ComputedStyleBase (Closed)

Created:
3 years, 11 months ago by sashab
Modified:
3 years, 11 months ago
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -6 lines) Patch
M third_party/WebKit/Source/build/scripts/make_computed_style_base.py View 1 2 3 4 5 7 chunks +56 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl View 1 2 3 1 chunk +9 lines, -0 lines 1 comment Download

Messages

Total messages: 28 (15 generated)
sashab
Thoughts welcome! :)
3 years, 11 months ago (2017-01-04 00:13:22 UTC) #4
meade_UTC10
lgtm nice! Good job making a reasonably short solution :) https://codereview.chromium.org/2611633003/diff/40001/third_party/WebKit/Source/build/scripts/make_computed_style_base.py File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2611633003/diff/40001/third_party/WebKit/Source/build/scripts/make_computed_style_base.py#newcode147 ...
3 years, 11 months ago (2017-01-04 00:43:32 UTC) #5
sashab
Thanks eddy - done :) Alexey - ptal at comment & new extra_padding_bytes variable to ...
3 years, 11 months ago (2017-01-05 00:05:19 UTC) #8
loyso (OOO)
On 2017/01/05 at 00:05:19, sashab wrote: > Thanks eddy - done :) > > Alexey ...
3 years, 11 months ago (2017-01-05 00:36:52 UTC) #9
aazzam
lgtm
3 years, 11 months ago (2017-01-05 00:38:16 UTC) #10
sashab
Removed reference to 32 bits. Thanks all for taking a look! :)
3 years, 11 months ago (2017-01-05 04:18:12 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2611633003/100001
3 years, 11 months ago (2017-01-05 04:18:32 UTC) #18
meade_UTC10
On 2017/01/05 00:36:52, loyso wrote: > On 2017/01/05 at 00:05:19, sashab wrote: > > Thanks ...
3 years, 11 months ago (2017-01-05 04:42:28 UTC) #19
loyso (OOO)
https://codereview.chromium.org/2611633003/diff/100001/third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl (right): https://codereview.chromium.org/2611633003/diff/100001/third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl#newcode15 third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl:15: ASSERT_SIZE(ComputedStyleBase, SameSizeAsComputedStyleBase); Out of curiosity.. Can we use just ...
3 years, 11 months ago (2017-01-05 05:18:17 UTC) #20
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/cc53ddbb49d378d18003915a0f2cded1a4dfea31
3 years, 11 months ago (2017-01-05 06:02:43 UTC) #23
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/b319a2f99e630380875c015a50fd20d5dd41ac3a Cr-Commit-Position: refs/heads/master@{#441594}
3 years, 11 months ago (2017-01-05 06:04:42 UTC) #25
sashab
Re meade - I'd rather not complicate the code with heaps of #ifdefs to figure ...
3 years, 11 months ago (2017-01-05 22:13:12 UTC) #26
sashab
3 years, 11 months ago (2017-01-10 01:52:53 UTC) #28
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. :)

Powered by Google App Engine
This is Rietveld 408576698