Web Inspector: Serious performance regression during continuous focused element style updates

[Alexander Pavlov (apavlov)]

Upstreaming http://code.google.com/p/chromium/issues/detail?id=82449

[Alexander Pavlov (apavlov)]

Created attachment 93912 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

Comment on attachment 93912 [details]
[PATCH] Suggested solution

Attachment 93912 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8708641

New failing tests:
inspector/styles/styles-disable-then-change.html
inspector/elements/elements-delete-inline-style.html
inspector/styles/styles-update-from-js.html

[WebKit Review Bot]

Created attachment 93950 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexander Pavlov (apavlov)]

Created attachment 94080 [details]
[PATCH] A different approach applied

[WebKit Review Bot]

Comment on attachment 94080 [details]
[PATCH] A different approach applied

Attachment 94080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8716355

[WebKit Review Bot]

Comment on attachment 94080 [details]
[PATCH] A different approach applied

Attachment 94080 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8718226

[Early Warning System Bot]

Comment on attachment 94080 [details]
[PATCH] A different approach applied

Attachment 94080 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8716358

[WebKit Review Bot]

Comment on attachment 94080 [details]
[PATCH] A different approach applied

Attachment 94080 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8719129

[Alexander Pavlov (apavlov)]

Created attachment 94214 [details]
[PATCH] Offline comments addressed

[Alexander Pavlov (apavlov)]

Created attachment 96567 [details]
[PATCH] Rebase

@reviewers: ping

[Pavel Feldman]

Comment on attachment 96567 [details]
[PATCH] Rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=96567&action=review

A number of small nits and a test request.

> Source/WebCore/inspector/Inspector.json:787
> +                "id": "DOMNodeAttributes",

Attributes. We can also rename DOMNode and DOMEventListener since type names have domain scope.

> Source/WebCore/inspector/Inspector.json:790
> +                    { "name": "id", "type": "integer", "description": "Node id." },

Element id to get attributes for.

> Source/WebCore/inspector/Inspector.json:987
> +                "name": "getNodeAttributes",

getAttributes

> Source/WebCore/inspector/Inspector.json:1013
> +                    { "name": "nodeId", "type": "integer", "description": "Id of the node that has changed." }

You might want to leave this as is for performance considerations.

> Source/WebCore/inspector/Inspector.json:1018
> +                "name": "inlineStyleInvalidated",

Please add a test for this new event.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:255
> +    m_domAgent->notifyStyleAttrInvalidated(elements);

StyleAttributeInvalidated (no abbreaviations in WebCore)

[Alexander Pavlov (apavlov)]

Comment on attachment 96567 [details]
[PATCH] Rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=96567&action=review

A fixed patch to be attached shortly

>> Source/WebCore/inspector/Inspector.json:787
>> +                "id": "DOMNodeAttributes",
> 
> Attributes. We can also rename DOMNode and DOMEventListener since type names have domain scope.

Done. I believe it's better to address the other renames in a separate change (in case we have to merge/roll back this or related changes).

>> Source/WebCore/inspector/Inspector.json:790
>> +                    { "name": "id", "type": "integer", "description": "Node id." },
> 
> Element id to get attributes for.

Done

>> Source/WebCore/inspector/Inspector.json:987
>> +                "name": "getNodeAttributes",
> 
> getAttributes

Done

>> Source/WebCore/inspector/Inspector.json:1013
>> +                    { "name": "nodeId", "type": "integer", "description": "Id of the node that has changed." }
> 
> You might want to leave this as is for performance considerations.

This is one of the main points of this change. With the Flickr demo, you get a constant high-volume traffic of updated attributes ("style" in particular) the Web Inspector is not coping with (parsing + handling).

>> Source/WebCore/inspector/Inspector.json:1018
>> +                "name": "inlineStyleInvalidated",
> 
> Please add a test for this new event.

Will do

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:255
>> +    m_domAgent->notifyStyleAttrInvalidated(elements);
> 
> StyleAttributeInvalidated (no abbreaviations in WebCore)

Done

[Alexander Pavlov (apavlov)]

Created attachment 97301 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Committed r88950: <http://trac.webkit.org/changeset/88950>

[Ryosuke Niwa]

inspector/styles/styles-update-from-js.html has been failing on GTK since this patch was landed:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/23400
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r88953%20(23400)/results.html

[Ryosuke Niwa]

+mrobinson since this patch caused a test failure on GTK.