Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2826633002: Exploit sharing when comparing and copying groups in ComputedStyle. (Closed)

Created:
1 year, 10 months ago by shend
Modified:
1 year, 9 months ago
Reviewers:
meade_UTC10, nainar
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Exploit sharing when comparing and copying groups in ComputedStyle. Groups inside ComputedStyle, such as StyleSurroundData, are stored as pointers. When we copy the StyleSurroundData of a ComputedStyle, instead of copying each member inside StyleSurroundData, we can copy the pointer itself and do a copy when we write to it (copy-on-write). Similarly, when we compare two StyleSurroundData pointers, we can first check if they're pointing to the same object. If so, then we know that the two StyleSurroundDatas are equal without having to compare each member inside StyleSurroundData. This patch introduces two macros: fieldwise_compare and fieldwise_copy, which generate the code to do this optimisation. These macros take a a list of fields to compare/copy and generate the cheapest set of code for that task, taking advantage of group sharing. This patch removes the perf cost of: https://codereview.chromium.org/2786883002 by generating the exact same code as what was originally handwritten. Diff of generated code: https://gist.github.com/darrnshn/fe97404e680016753030fc788fcf24ff/revisions BUG=628043 Review-Url: https://codereview.chromium.org/2826633002 Cr-Commit-Position: refs/heads/master@{#467243} Committed: https://chromium.googlesource.com/chromium/src/+/51ad943c5fac355ab0166e1f47a104e8bc0b3bc6

Patch Set 1 #

Patch Set 2 : Fix #

Patch Set 3 : Fix #

Patch Set 4 : Description #

Patch Set 5 : Add comments #

Total comments: 2

Patch Set 6 : Fix #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Patch Set 10 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -23 lines) Patch
M third_party/WebKit/Source/build/scripts/make_computed_style_base.py View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/template_expander.py View 1 2 chunks +5 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.cpp.tmpl View 1 2 3 4 5 6 2 chunks +11 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl View 1 2 3 4 5 6 7 8 3 chunks +19 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl View 1 2 3 4 5 6 7 1 chunk +33 lines, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 39 (32 generated)
shend
Hi Naina, PTAL. The macro is a bit complicated, lemme know what I can do ...
1 year, 10 months ago (2017-04-19 04:16:41 UTC) #8
nainar
lgtm with the nit https://codereview.chromium.org/2826633002/diff/80001/third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826633002/diff/80001/third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl#newcode78 third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:78: {{getter_expression(field)}} = other.{{getter_expression(field)}}; setter_expression(field)
1 year, 10 months ago (2017-04-19 04:46:39 UTC) #9
shend
https://codereview.chromium.org/2826633002/diff/80001/third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl File third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl (right): https://codereview.chromium.org/2826633002/diff/80001/third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl#newcode78 third_party/WebKit/Source/build/scripts/templates/fields/field.tmpl:78: {{getter_expression(field)}} = other.{{getter_expression(field)}}; On 2017/04/19 at 04:46:39, nainar wrote: ...
1 year, 10 months ago (2017-04-20 00:17:33 UTC) #10
shend
Hi Eddy, this is a patch that I'll land together with groups. The macro is ...
1 year, 10 months ago (2017-04-20 03:39:00 UTC) #12
meade_UTC10
lgtm
1 year, 9 months ago (2017-04-24 07:10:21 UTC) #29
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/2826633002/180001
1 year, 9 months ago (2017-04-26 05:20:55 UTC) #36
commit-bot: I haz the power
1 year, 9 months ago (2017-04-26 05:26:38 UTC) #39
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/51ad943c5fac355ab0166e1f47a1...

Powered by Google App Engine
This is Rietveld 408576698