Bug 82235 - Speed up updates of existing CSS properties from JS.
Summary: Speed up updates of existing CSS properties from JS.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-26 12:41 PDT by Alexis Menard (darktears)
Modified: 2012-03-28 05:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2012-03-26 12:43 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (9.63 MB, application/zip)
2012-03-26 13:22 PDT, WebKit Review Bot
no flags Details
Patch (16.11 KB, patch)
2012-03-26 15:38 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (14.19 KB, patch)
2012-03-28 04:56 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-03-26 12:41:01 PDT
Speed up updates of existing CSS properties from JS.
Comment 1 Alexis Menard (darktears) 2012-03-26 12:43:56 PDT
Created attachment 133868 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-26 13:22:23 PDT
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
Comment 3 WebKit Review Bot 2012-03-26 13:22:30 PDT
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
Comment 4 Alexis Menard (darktears) 2012-03-26 13:44:42 PDT
(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?
Comment 5 Benjamin Poulain 2012-03-26 15:04:47 PDT
> 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.
Comment 6 Alexis Menard (darktears) 2012-03-26 15:38:00 PDT
Created attachment 133908 [details]
Patch
Comment 7 Alexis Menard (darktears) 2012-03-27 03:48:18 PDT
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
Comment 8 Andreas Kling 2012-03-27 04:28:43 PDT
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. :)
Comment 9 Alexis Menard (darktears) 2012-03-27 04:52:22 PDT
(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.
Comment 10 Alexis Menard (darktears) 2012-03-28 04:56:03 PDT
Created attachment 134268 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-03-28 05:40:54 PDT
Comment on attachment 134268 [details]
Patch for landing

Clearing flags on attachment: 134268

Committed r112387: <http://trac.webkit.org/changeset/112387>
Comment 12 WebKit Review Bot 2012-03-28 05:41:11 PDT
All reviewed patches have been landed.  Closing bug.