Alternate computation of axObjectCacheOwner to improve perf

Alternate computation of axObjectCacheOwner to improve perf

BUG=571302, 510410, 532249
Committed: https://crrev.com/77ffbd8f10c13590ff68170600284a10a634c410
Cr-Commit-Position: refs/heads/master@{#367039}

[dmazzoni, 2015-12-22 05:55:23]

Description was changed from

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302
==========

to

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302,510410,532249
==========

[dmazzoni, 2015-12-22 05:57:23]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org, mkwst@chromium.org

[dmazzoni, 2015-12-22 05:57:23]

See bug 571302 for context; the original patch (crrev.com/1525893002) regressed
performance, apparently because calling localFrameRoot() is more expensive than
walking up the chain of frame owner elements.

[dcheng, 2015-12-28 22:12:19]

lgtm

https://codereview.chromium.org/1544443004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1544443004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:2318: while (frame &&
frame->owner() && frame->owner()->isLocal()) {
Nit: add a comment so someone (e.g. me) doesn't see this and think "hmm, why
don't I just use localFrameRoot()...)

[dmazzoni, 2015-12-29 00:13:08]

https://codereview.chromium.org/1544443004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1544443004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:2318: while (frame &&
frame->owner() && frame->owner()->isLocal()) {
On 2015/12/28 22:12:19, dcheng wrote:
> Nit: add a comment so someone (e.g. me) doesn't see this and think "hmm, why
> don't I just use localFrameRoot()...)

Done.

[dmazzoni, 2015-12-29 00:13:32]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-29 00:13:32]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1544443004/#ps20001
(title: "Add comment")

[commit-bot: I haz the power, 2015-12-29 00:13:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544443004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544443004/20001

[commit-bot: I haz the power, 2015-12-29 02:16:04]

Description was changed from

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302,510410,532249
==========

to

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302,510410,532249
==========

[commit-bot: I haz the power, 2015-12-29 02:16:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-29 02:17:21]

Description was changed from

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302,510410,532249
==========

to

==========
Alternate computation of axObjectCacheOwner to improve perf

BUG=571302,510410,532249

Committed: https://crrev.com/77ffbd8f10c13590ff68170600284a10a634c410
Cr-Commit-Position: refs/heads/master@{#367039}
==========

[commit-bot: I haz the power, 2015-12-29 02:17:21]

Patchset 2 (id:??) landed as
https://crrev.com/77ffbd8f10c13590ff68170600284a10a634c410
Cr-Commit-Position: refs/heads/master@{#367039}

[esprehn, 2015-12-29 21:20:00]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-12-29 21:20:01]

Why is this faster? What benchmarks should move? How do we fix localFrameRoot to
be this fast?

Please write better change descriptions. :)

This hack doesn't seem like a good idea, we should just fix localFrameRoot.

[dcheng, 2015-12-29 21:21:55]

On 2015/12/29 at 21:20:01, esprehn wrote:
> Why is this faster? What benchmarks should move? How do we fix localFrameRoot
to be this fast?
> 
> Please write better change descriptions. :)
> 
> This hack doesn't seem like a good idea, we should just fix localFrameRoot.

If I had to guess, it's probably because there's more virtual calls when calling
localFrameRoot, since we have to go from core -> web types and then back.

[dmazzoni, 2015-12-29 21:28:22]

I wasn't sure if it'd be useful to include all of this in the change description
since it's in the linked bug (http://crbug.com/571302).

Test Command: python src/tools/perf/run_benchmark -v --browser=release
--output-format=chartjson --also-run-disabled-tests blink_perf.dom
Test Metric: select-single-add/select-single-add
Relative Change: 12.65%

I don't see an obvious way to fix localFrameRoot. It's not doing anything
stupid.

I'm working on eliminating axObjectCacheOwner() completely by switching to one
AXObjectCache per document, that will improve perf even more.
http://crbug.com/532249.

[dcheng, 2015-12-29 21:29:46]

On 2015/12/29 at 21:28:22, dmazzoni wrote:
> I wasn't sure if it'd be useful to include all of this in the change
description since it's in the linked bug (http://crbug.com/571302).
> 
> Test Command: python src/tools/perf/run_benchmark -v --browser=release
--output-format=chartjson --also-run-disabled-tests blink_perf.dom
> Test Metric: select-single-add/select-single-add
> Relative Change: 12.65%
> 
> I don't see an obvious way to fix localFrameRoot. It's not doing anything
stupid.
> 
> I'm working on eliminating axObjectCacheOwner() completely by switching to one
AXObjectCache per document, that will improve perf even more.
http://crbug.com/532249.

I think merging core/web will be the primary thing that helps here.

[esprehn, 2015-12-29 22:39:11]

In the future please duplicate that into the change description. You always
have your git checkout, even offline, the bug tracker is not a replacement
for good change descriptions. :)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-12-29 22:39:12]

In the future please duplicate that into the change description. You always
have your git checkout, even offline, the bug tracker is not a replacement
for good change descriptions. :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.