Bug 82235 – Speed up updates of existing CSS properties from JS.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

Speed up updates of existing CSS properties from JS.

[Alexis Menard (darktears)]

Created attachment 133868 [details]
Patch

[WebKit Review Bot]

Comment on attachment 133868 [details]
Patch

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

New failing tests:
fast/events/ondrop-text-html.html
inspector/styles/styles-update-from-js.html
inspector/styles/styles-new-API.html
editing/pasteboard/onpaste-text-html.html
fast/mutation/observe-attributes.html
editing/pasteboard/data-transfer-items.html
fast/dom/css-set-property-exception.html

[WebKit Review Bot]

Created attachment 133877 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

(In reply to comment #2)
> (From update of attachment 133868 [details])
> Attachment 133868 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12134998
> 
> New failing tests:
> fast/events/ondrop-text-html.html
> inspector/styles/styles-update-from-js.html
> inspector/styles/styles-new-API.html
> editing/pasteboard/onpaste-text-html.html
> fast/mutation/observe-attributes.html
> editing/pasteboard/data-transfer-items.html
> fast/dom/css-set-property-exception.html

All these failure are because the updated properties are not added at the end of the list of properties therefore the cssText is different. You may say I broke existing behavior but then I ran fast/dom/css-set-property-exception.html in both Firefox 11 and Opera.

Firefox 11 returns the same order as I return with the patch (meaning that the updated CSS property is not at the end of the list but I update the current value)

Opera returns the current behavior, the updated property (e.g. display) is at the end.

Therefore it's not safe to rely on that order.

Before I update or not the expected output any opinion?

[Benjamin Poulain]

> Before I update or not the expected output any opinion?

I think it is ok. If the other browser do not enforce any specific order, we do not break any compatibility.

[Alexis Menard (darktears)]

Created attachment 133908 [details]
Patch

[Alexis Menard (darktears)]

Modified version of CSSPropertySetterGetter

PerfTestRunner.run(function() {
    for (key in properties) {
        var value = div.style[key];
        //div.style[key] = "";
        div.style[key] = properties[key];
    }
}, 5000);


Without the patch :

darktears@DarthVader:~/dev/troll/webkit$ WEBKITOUTPUTDIR=/home/darktears/dev/troll/webkit2-trunk run-perf-tests --platform=qt PerformanceTests/CSS/CSSPropertySetterGetter.html
Running CSS/CSSPropertySetterGetter.html (1 of 1)
RESULT CSS: CSSPropertySetterGetter= 940.65 ms
median= 940.5 ms, stdev= 1.15217186218 ms, min= 939.0 ms, max= 943.0 ms
Finished: 20.161777 s

WEBKITOUTPUTDIR=/home/darktears/dev/troll/webkit2-trunk run-perf-tests    20.16s user 0.06s system 99% cpu 20.408 total

With the patch :

darktears@DarthVader:~/dev/troll/webkit$ WEBKITOUTPUTDIR=/home/darktears/dev/troll/webkit2-trunk run-perf-tests --platform=qt PerformanceTests/CSS/CSSPropertySetterGetter.html
Running CSS/CSSPropertySetterGetter.html (1 of 1)
RESULT CSS: CSSPropertySetterGetter= 689.75 ms
median= 689.0 ms, stdev= 2.11837201643 ms, min= 688.0 ms, max= 697.0 ms
Finished: 14.896826 s

WEBKITOUTPUTDIR=/home/darktears/dev/troll/webkit2-trunk run-perf-tests    14.92s user 0.06s system 98% cpu 15.163 total

[Andreas Kling]

Comment on attachment 133908 [details]
Patch

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

Great idea. r=me

> Source/WebCore/ChangeLog:10
> +        Improve the way we handle updating an existing CSS property by replacing its value
> +        by the new one rather than removing the old value and then adding the new one. This
> +        speed up the update of existing properties by 28%.

Is this a 28% improvement on a test we have in PerformanceTests/? If so, you should mention it by name. :)

[Alexis Menard (darktears)]

(In reply to comment #8)
> (From update of attachment 133908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133908&action=review
> 
> Great idea. r=me
> 
> > Source/WebCore/ChangeLog:10
> > +        Improve the way we handle updating an existing CSS property by replacing its value
> > +        by the new one rather than removing the old value and then adding the new one. This
> > +        speed up the update of existing properties by 28%.
> 
> Is this a 28% improvement on a test we have in PerformanceTests/? If so, you should mention it by name. :)

I have added https://bugs.webkit.org/show_bug.cgi?id=82321 which covers the performance improvement.

[Alexis Menard (darktears)]

Created attachment 134268 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 134268 [details]
Patch for landing

Clearing flags on attachment: 134268

Committed r112387: <http://trac.webkit.org/changeset/112387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.