Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1636503005:
Avoid unnecessary CoW for outer DataRef when applying transform-origin (Closed)
|
Created:
4 years, 11 months ago by fs Modified:
4 years, 11 months ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid unnecessary CoW for outer DataRef when applying transform-origin
Even if the transform-origin is the same as the current value,
rareNonInheritedData will still be copied.
Add a new macro SET_NESTED_VAR, similar to the existing SET_VAR, but
allowing an intermediate |base| field to be specified, avoiding the
access in the dereference of that field from the group.
BUG=571183
Committed: https://crrev.com/2e0ff75609854844e4e83deae140b728535fea23
Cr-Commit-Position: refs/heads/master@{#371766}
Patch Set 1 #
Messages
Total messages: 15 (4 generated)
fs@opera.com changed reviewers: + pdr@chromium.org
Something I noticed while looking at the reference perf-bug. Took it for a spin on the perf-bots: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... (The new macro can probably be used for more values.)
Description was changed from ========== Avoid unnecessary CoW for outer DataRef when setting transform-origin Even if the transform-origin is the same as the current value, rareNonInheritedData will still be copied. Add a new macro SET_NESTED_VAR, similar to the existing SET_VAR, but allowing an intermediate |base| field to be specified, avoiding the access in the dereference of that field from the group. BUG=571183 ========== to ========== Avoid unnecessary CoW for outer DataRef when applying transform-origin Even if the transform-origin is the same as the current value, rareNonInheritedData will still be copied. Add a new macro SET_NESTED_VAR, similar to the existing SET_VAR, but allowing an intermediate |base| field to be specified, avoiding the access in the dereference of that field from the group. BUG=571183 ==========
On 2016/01/26 at 18:29:45, fs wrote: > Something I noticed while looking at the reference perf-bug. > > Took it for a spin on the perf-bots: > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > (The new macro can probably be used for more values.) This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers?
On 2016/01/26 at 18:52:59, pdr wrote: > On 2016/01/26 at 18:29:45, fs wrote: > > Something I noticed while looking at the reference perf-bug. > > > > Took it for a spin on the perf-bots: > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > (The new macro can probably be used for more values.) > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.)
On 2016/01/26 at 19:04:40, fs wrote: > On 2016/01/26 at 18:52:59, pdr wrote: > > On 2016/01/26 at 18:29:45, fs wrote: > > > Something I noticed while looking at the reference perf-bug. > > > > > > Took it for a spin on the perf-bots: > > > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > > > (The new macro can probably be used for more values.) > > > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? > > Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.) Are we looking at the same link? That just shows 0 for me. LGTM based on your local results though :) Can you file a bug to do this for more properties?
On 2016/01/26 at 19:07:18, pdr wrote: > On 2016/01/26 at 19:04:40, fs wrote: > > On 2016/01/26 at 18:52:59, pdr wrote: > > > On 2016/01/26 at 18:29:45, fs wrote: > > > > Something I noticed while looking at the reference perf-bug. > > > > > > > > Took it for a spin on the perf-bots: > > > > > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > > > > > (The new macro can probably be used for more values.) > > > > > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? > > > > Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.) > > Are we looking at the same link? That just shows 0 for me. Now I'm confused (and intrigued...). This is the try CL: https://codereview.chromium.org/1638893002 > LGTM based on your local results though :) Can you file a bug to do this for more properties? Sure. Filed crbug.com/581413.
On 2016/01/26 at 19:17:10, fs wrote: > On 2016/01/26 at 19:07:18, pdr wrote: > > On 2016/01/26 at 19:04:40, fs wrote: > > > On 2016/01/26 at 18:52:59, pdr wrote: > > > > On 2016/01/26 at 18:29:45, fs wrote: > > > > > Something I noticed while looking at the reference perf-bug. > > > > > > > > > > Took it for a spin on the perf-bots: > > > > > > > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > > > > > > > (The new macro can probably be used for more values.) > > > > > > > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? > > > > > > Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.) > > > > Are we looking at the same link? That just shows 0 for me. > > Now I'm confused (and intrigued...). This is the try CL: https://codereview.chromium.org/1638893002 > > > LGTM based on your local results though :) Can you file a bug to do this for more properties? > > Sure. Filed crbug.com/581413. This is strange. It looks like the results are incorrectly filed under "memory", even though they are not memory results. Do you see this behavior too? Try toggling the "time | memory | all" switch.
On 2016/01/26 at 19:33:15, pdr wrote: > On 2016/01/26 at 19:17:10, fs wrote: > > On 2016/01/26 at 19:07:18, pdr wrote: > > > On 2016/01/26 at 19:04:40, fs wrote: > > > > On 2016/01/26 at 18:52:59, pdr wrote: > > > > > On 2016/01/26 at 18:29:45, fs wrote: > > > > > > Something I noticed while looking at the reference perf-bug. > > > > > > > > > > > > Took it for a spin on the perf-bots: > > > > > > > > > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > > > > > > > > > (The new macro can probably be used for more values.) > > > > > > > > > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? > > > > > > > > Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.) > > > > > > Are we looking at the same link? That just shows 0 for me. > > > > Now I'm confused (and intrigued...). This is the try CL: https://codereview.chromium.org/1638893002 > > > > > LGTM based on your local results though :) Can you file a bug to do this for more properties? > > > > Sure. Filed crbug.com/581413. > > This is strange. It looks like the results are incorrectly filed under "memory", even though they are not memory results. Do you see this behavior too? Try toggling the "time | memory | all" switch. Yes, that's what I see as well (which was confusing at first...)
On 2016/01/26 at 19:37:32, fs wrote: > On 2016/01/26 at 19:33:15, pdr wrote: > > On 2016/01/26 at 19:17:10, fs wrote: > > > On 2016/01/26 at 19:07:18, pdr wrote: > > > > On 2016/01/26 at 19:04:40, fs wrote: > > > > > On 2016/01/26 at 18:52:59, pdr wrote: > > > > > > On 2016/01/26 at 18:29:45, fs wrote: > > > > > > > Something I noticed while looking at the reference perf-bug. > > > > > > > > > > > > > > Took it for a spin on the perf-bots: > > > > > > > > > > > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-01... > > > > > > > > > > > > > > (The new macro can probably be used for more values.) > > > > > > > > > > > > This looks great I'd like to make sure it works before landing it. The perf numbers show zero for me :/ Were you able to verify this locally? If so, can you share the numbers? > > > > > > > > > > Hmm, zero even if selecting "All"? I only ran this for Cowboy (mentioned in the bug) locally and got similar results as the bot (avg 74->71 IIRC.) > > > > > > > > Are we looking at the same link? That just shows 0 for me. > > > > > > Now I'm confused (and intrigued...). This is the try CL: https://codereview.chromium.org/1638893002 > > > > > > > LGTM based on your local results though :) Can you file a bug to do this for more properties? > > > > > > Sure. Filed crbug.com/581413. > > > > This is strange. It looks like the results are incorrectly filed under "memory", even though they are not memory results. Do you see this behavior too? Try toggling the "time | memory | all" switch. > > Yes, that's what I see as well (which was confusing at first...) Ah, this was my fault. I misunderstood when you said 'Hmm, zero even if selecting "All"'. Selecting All does indeed work.
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1636503005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1636503005/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary CoW for outer DataRef when applying transform-origin Even if the transform-origin is the same as the current value, rareNonInheritedData will still be copied. Add a new macro SET_NESTED_VAR, similar to the existing SET_VAR, but allowing an intermediate |base| field to be specified, avoiding the access in the dereference of that field from the group. BUG=571183 ========== to ========== Avoid unnecessary CoW for outer DataRef when applying transform-origin Even if the transform-origin is the same as the current value, rareNonInheritedData will still be copied. Add a new macro SET_NESTED_VAR, similar to the existing SET_VAR, but allowing an intermediate |base| field to be specified, avoiding the access in the dereference of that field from the group. BUG=571183 Committed: https://crrev.com/2e0ff75609854844e4e83deae140b728535fea23 Cr-Commit-Position: refs/heads/master@{#371766} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2e0ff75609854844e4e83deae140b728535fea23 Cr-Commit-Position: refs/heads/master@{#371766} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
