Bug 140753 - Poor performance on IE's Chalkboard benchmark
Summary: Poor performance on IE's Chalkboard benchmark
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-21 19:19 PST by Said Abou-Hallawa
Modified: 2015-01-28 21:23 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.72 KB, patch)
2015-01-21 19:58 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (921.79 KB, application/zip)
2015-01-21 20:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (1.02 MB, application/zip)
2015-01-21 20:52 PST, Build Bot
no flags Details
Patch (14.92 KB, patch)
2015-01-22 21:24 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2015-01-23 10:54 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.05 KB, patch)
2015-01-23 17:05 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.49 KB, patch)
2015-01-23 18:17 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.98 KB, patch)
2015-01-23 19:24 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.20 KB, patch)
2015-01-27 13:22 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.42 KB, patch)
2015-01-27 20:44 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2015-01-28 17:52 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2015-01-28 18:57 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-01-21 19:19:38 PST
On the same machine, the benchmark http://ie.microsoft.com/testdrive/Performance/Chalkboard/ is tested with different window sizes:

Window size (2560 x 1260):
Safari 10600.3.14: 70.60 sec
FireFox: 120.40 sec
Chrome: 26.21

Window size (800 x 600):
Safari 10600.3.14: 6.49 sec
FireFox: 98.74 sec
Chrome: 8.14 sec

The reason for having WebKit slower on this benchmark, with larger widow size, is the SVG rendering code does not skip the SVG elements which are outside the clipping rectangle. We draw the all SVG elements even if some of them are completely outside the clipping rectangle. This is very costly with complex primitives like the <path>. The fix is to propagate the correct clipping rectangle in the PaintingInfo when passing it to the SVGShape::paint().
Comment 1 Said Abou-Hallawa 2015-01-21 19:26:16 PST
The same issue was reported in the past: https://bugs.webkit.org/show_bug.cgi?id=104693. But it looks like it was happening because of SVGImageCache and other stuff and it was resolved because the benchmark completes in 9sec with 1024x1024 screen. A performance test needs to be written for this benchmark to track if it's slowed down in the future.
Comment 2 Said Abou-Hallawa 2015-01-21 19:58:54 PST
Created attachment 245109 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-01-21 20:02:30 PST
Safari completes this benchmark on a window size 2560 x 1260 in:

Without the patch: 70.60 sec
With the patch: 10.35 sec
Comment 4 Build Bot 2015-01-21 20:46:18 PST
Comment on attachment 245109 [details]
Patch

Attachment 245109 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5067252034437120

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
svg/custom/anchor-on-use.svg
fast/sub-pixel/zoomed-image-tiles.html
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 5 Build Bot 2015-01-21 20:46:21 PST
Created attachment 245114 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-01-21 20:52:09 PST
Comment on attachment 245109 [details]
Patch

Attachment 245109 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6224187374960640

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
svg/custom/anchor-on-use.svg
fast/sub-pixel/zoomed-image-tiles.html
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 7 Build Bot 2015-01-21 20:52:11 PST
Created attachment 245115 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 Simon Fraser (smfr) 2015-01-22 11:23:18 PST
Comment on attachment 245109 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:252
> +    view->paint(context, context->clipBounds().isEmpty() ? enclosingIntRect(srcRect) : context->clipBounds());

You shouldn't call clipBounds() twice; it's not that cheap.
Comment 9 Said Abou-Hallawa 2015-01-22 21:24:24 PST
Created attachment 245204 [details]
Patch
Comment 10 Said Abou-Hallawa 2015-01-23 09:15:45 PST
(In reply to comment #8)
> Comment on attachment 245109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245109&action=review
> 
> > Source/WebCore/svg/graphics/SVGImage.cpp:252
> > +    view->paint(context, context->clipBounds().isEmpty() ? enclosingIntRect(srcRect) : context->clipBounds());
> 
> You shouldn't call clipBounds() twice; it's not that cheap.

Fixed
Comment 11 Said Abou-Hallawa 2015-01-23 10:54:39 PST
Created attachment 245239 [details]
Patch
Comment 12 Said Abou-Hallawa 2015-01-23 17:05:19 PST
Created attachment 245264 [details]
Patch
Comment 13 Said Abou-Hallawa 2015-01-23 17:10:41 PST
Sometimes when running run-perf-tests from the command line, I get FAILED results after very long time.

Running 1 tests
Running SVG/UnderTheSeeBenchmark.html (1 of 1)
timeout: SVG/UnderTheSeeBenchmark.html
FAILED
Finished: 605.008695 s

When I used "-2" option I got the following results:

Running 1 tests
Running SVG/UnderTheSeeBenchmark.html (1 of 1)
error: SVG/UnderTheSeeBenchmark.html
2015-01-23 16:44:16.142 com.apple.WebKit.Networking.Development[71068:24399801] NO NSURLStorage client.  Cannot set min-size for VM cached resources.

But when opening the test in the browser, it runs really fast and finishes normally. I am not sure if I am doing a mistake in the test or I am passing the wrong options to run-perf-tests.
Comment 14 Simon Fraser (smfr) 2015-01-23 17:14:32 PST
We may need to fix DRT and WTR to use onscreen windows during perf testing.
Comment 15 Said Abou-Hallawa 2015-01-23 18:17:33 PST
Created attachment 245269 [details]
Patch
Comment 16 Said Abou-Hallawa 2015-01-23 18:21:21 PST
I changed the tests to take less time be having the number of target rectangles be "1". On my machine WorldcupBenchmark.html takes 14 seconds and UnderTheSeeBenchmark.html takes 80 seconds. I think the patch is now ready for review.
Comment 17 Said Abou-Hallawa 2015-01-23 19:24:12 PST
Created attachment 245272 [details]
Patch
Comment 18 Said Abou-Hallawa 2015-01-23 19:28:19 PST
I removed the browser targets so the test moves the image to a single target.
Comment 19 Ryosuke Niwa 2015-01-23 20:12:02 PST
Comment on attachment 245269 [details]
Patch

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

> PerformanceTests/SVG/resources/RenderAnimate.js:56
> +var globals = {

Why don't we create a class instead?  Having all properties in one object and accessing it from various functions seems like a code smell.

> PerformanceTests/SVG/resources/RenderAnimate.js:66
> +    image    : document.getElementById("Image"),
> +    timer    : document.getElementById("Timer"),
> +    targets  : window.testRunner ? performanceTargets : browserTargets,
> +    index    : 0,
> +    delay    : window.testRunner ? 0 : 1000,
> +    duration : { start : null, end : null },
> +    window   : { w: 0, h: 0 },
> +    source   : { w: 0, h: 0 },
> +    target   : { x: 0, y: 0, w: 0, h: 0 },
> +    current  : { x: 0, y: 0, w: 0, h: 0 },

Please don't align : like this per our style guideline.  Also, spell out width and height.

> PerformanceTests/SVG/resources/RenderAnimate.js:69
> +// Performance test controller

This comment repeats what the code says. Please remove.

> PerformanceTests/SVG/resources/RenderAnimate.js:74
> +        // We have to remove all the elements in the page when the test ends. Otherwise the
> +        // run-perf-test will say it failed

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:82
> +// The entry point of the test

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:87
> +    // Break the loop if the test is teared down
> +    if (globals.image == null)
> +        return;

We shouldn't need this code. measureValueAsync returns false when we're done.

> PerformanceTests/SVG/resources/RenderAnimate.js:91
> +    // setupTest() only once
> +    if (globals.window.w == 0)
> +        setupTest();

Why don't we invert the relationship between two functions so that setupTest will call startTest at the end?

> PerformanceTests/SVG/resources/RenderAnimate.js:93
> +    setupRun();

Why does this need to be a separate function?
It seems like it's not called anywhere else and "setupRun" doesn't convey any extra information "startTest" or startTest doesn't.

> PerformanceTests/SVG/resources/RenderAnimate.js:105
> +    // Store the original image zize

This comment repeats the code. Please remove it.

> PerformanceTests/SVG/resources/RenderAnimate.js:126
> +    globals.target.x  = globals.current.x;
> +    globals.target.y  = globals.current.y;
> +    globals.target.w  = globals.current.w;
> +    globals.target.h  = globals.current.h;
> +
> +    globals.index          = 0;
> +    globals.duration.start = null;
> +    globals.duration.end   = null;

We shouldn't align = signs like this per our style guide.

> PerformanceTests/SVG/resources/RenderAnimate.js:130
> +    globals.image.style.visibility = "visible";
> +    globals.image.style.opacity    = 1;
> +    globals.timer.style.opacity    = 1;

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:144
> +    if (++globals.index < globals.targets.length) {

We prefer early return so it's better to write this as:
globals.index++;
if (globals.index >= globals.targets.length) {
  setTimeout(nextRun, globals.delay);
  return;
}
... (no else).

> PerformanceTests/SVG/resources/RenderAnimate.js:175
> +            && Math.abs(globals.current.y - globals.target.y) < 0.01 
> +            && Math.abs(globals.current.w - globals.target.w) < 0.01) {

Wrong indentation.  && should appear exactly 4 spaces to the right of "if (".

> PerformanceTests/SVG/resources/RenderAnimate.js:176
> +        // If we we are vert close to the target, move to the next target

Instead of saying this in comment, allocate a descriptive local variable such as "approximatelyAtTarget".

> PerformanceTests/SVG/resources/RenderAnimate.js:179
> +        // Keep moving to the target rectangle gradually

This comment repeats the code. Remove.

> PerformanceTests/SVG/resources/RenderAnimate.js:190
> +    globals.image.style.width = globals.current.w + "px";
> +    globals.image.style.left  = globals.current.x + "px";
> +    globals.image.style.top   = globals.current.y + "px";

Ditto about aligning = signs.

> PerformanceTests/SVG/resources/RenderAnimate.js:193
> +function nextRun()

I would call this function recordResultAndScheduleNewRun or something since it does a lot more than just scheduling the next run.
Comment 20 Said Abou-Hallawa 2015-01-27 13:22:35 PST
Created attachment 245464 [details]
Patch
Comment 21 Said Abou-Hallawa 2015-01-27 13:51:33 PST
(In reply to comment #19)
> Comment on attachment 245269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245269&action=review
> 
> > PerformanceTests/SVG/resources/RenderAnimate.js:56
> > +var globals = {
> 
> Why don't we create a class instead?  Having all properties in one object
> and accessing it from various functions seems like a code smell.
> 

Done. Thanks for the advice.

> > PerformanceTests/SVG/resources/RenderAnimate.js:66
> > +    image    : document.getElementById("Image"),
> > +    timer    : document.getElementById("Timer"),
> > +    targets  : window.testRunner ? performanceTargets : browserTargets,
> > +    index    : 0,
> > +    delay    : window.testRunner ? 0 : 1000,
> > +    duration : { start : null, end : null },
> > +    window   : { w: 0, h: 0 },
> > +    source   : { w: 0, h: 0 },
> > +    target   : { x: 0, y: 0, w: 0, h: 0 },
> > +    current  : { x: 0, y: 0, w: 0, h: 0 },
> 
> Please don't align : like this per our style guideline.  Also, spell out
> width and height.
> 

The script is almost rewritten but the new one does not have any alignment for colons.

> > PerformanceTests/SVG/resources/RenderAnimate.js:69
> > +// Performance test controller
> 
> This comment repeats what the code says. Please remove.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:74
> > +        // We have to remove all the elements in the page when the test ends. Otherwise the
> > +        // run-perf-test will say it failed
> 
> Ditto.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:82
> > +// The entry point of the test
> 
> Ditto.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:87
> > +    // Break the loop if the test is teared down
> > +    if (globals.image == null)
> > +        return;
> 
> We shouldn't need this code. measureValueAsync returns false when we're done.
> 

Done. The return value of measureValueAsync() controls when we should exit the test.

> > PerformanceTests/SVG/resources/RenderAnimate.js:91
> > +    // setupTest() only once
> > +    if (globals.window.w == 0)
> > +        setupTest();
> 
> Why don't we invert the relationship between two functions so that setupTest
> will call startTest at the end?
> 

The structure of the test is now different and I think the new one is clearer.

> > PerformanceTests/SVG/resources/RenderAnimate.js:93
> > +    setupRun();
> 
> Why does this need to be a separate function?
> It seems like it's not called anywhere else and "setupRun" doesn't convey
> any extra information "startTest" or startTest doesn't.
> 

The structure of the test is now different and I think the new one is clearer.

> > PerformanceTests/SVG/resources/RenderAnimate.js:105
> > +    // Store the original image zize
> 
> This comment repeats the code. Please remove it.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:126
> > +    globals.target.x  = globals.current.x;
> > +    globals.target.y  = globals.current.y;
> > +    globals.target.w  = globals.current.w;
> > +    globals.target.h  = globals.current.h;
> > +
> > +    globals.index          = 0;
> > +    globals.duration.start = null;
> > +    globals.duration.end   = null;
> 
> We shouldn't align = signs like this per our style guide.
> 

Alignment is removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:130
> > +    globals.image.style.visibility = "visible";
> > +    globals.image.style.opacity    = 1;
> > +    globals.timer.style.opacity    = 1;
> 
> Ditto.
> 

Alignment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:144
> > +    if (++globals.index < globals.targets.length) {
> 
> We prefer early return so it's better to write this as:
> globals.index++;
> if (globals.index >= globals.targets.length) {
>   setTimeout(nextRun, globals.delay);
>   return;
> }
> ... (no else).
> 

Early return was done.

> > PerformanceTests/SVG/resources/RenderAnimate.js:175
> > +            && Math.abs(globals.current.y - globals.target.y) < 0.01 
> > +            && Math.abs(globals.current.w - globals.target.w) < 0.01) {
> 
> Wrong indentation.  && should appear exactly 4 spaces to the right of "if (".
> 

This code was removed and rewritten in a different way but there is no need for alignment in the new code.

> > PerformanceTests/SVG/resources/RenderAnimate.js:176
> > +        // If we we are vert close to the target, move to the next target
> 
> Instead of saying this in comment, allocate a descriptive local variable
> such as "approximatelyAtTarget".
> 

Done. A new object is defined 'Rectangle' and this object has a method called isAlmostEqual().

> > PerformanceTests/SVG/resources/RenderAnimate.js:179
> > +        // Keep moving to the target rectangle gradually
> 
> This comment repeats the code. Remove.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:190
> > +    globals.image.style.width = globals.current.w + "px";
> > +    globals.image.style.left  = globals.current.x + "px";
> > +    globals.image.style.top   = globals.current.y + "px";
> 
> Ditto about aligning = signs.
> 

Alignment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:193
> > +function nextRun()
> 
> I would call this function recordResultAndScheduleNewRun or something since
> it does a lot more than just scheduling the next run.

The structure of the test is now different and I think the new one is clearer.
Comment 22 Ryosuke Niwa 2015-01-27 14:14:35 PST
Comment on attachment 245464 [details]
Patch

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

> PerformanceTests/SVG/resources/RenderAnimator.js:3
> +//
> +// This code is used to run an SVG benchmark through the WebKit performance test infrastructure.
> +//

This seems like an obvious point that doesn't deserve a comment.

> PerformanceTests/SVG/resources/RenderAnimator.js:5
> +//   1. Include it at the end of the body tag in the HTML

Is this really a requirement?  Can't we just wait for DOMContentLoaded for example?

> PerformanceTests/SVG/resources/RenderAnimator.js:6
> +//   2. The HTML has only two elements whose IDs are "Image" and "Timer".

Can we just insert "Timer" element right after the image element automatically instead?

> PerformanceTests/SVG/resources/RenderAnimator.js:17
> +function bind(f, obj) {
> +    return function() {
> +        return f.apply(obj, arguments);
> +    };
> +}

JavaScript has builtin "bind".  e.g. f.bind(obj).apply(arguments).

> PerformanceTests/SVG/resources/RenderAnimator.js:69
> +function ElapsedTimer() {

I wouldn't call this a timer since it wouldn't schedule anything.
How about ElapsedTime?

> PerformanceTests/SVG/resources/RenderAnimator.js:131
> +    this.initialize = function() {
> +        _animateMoveIndex = 0;
> +        this.nextTargetRect();
> +        _animateRect = _targetRect;
> +    };

Why can't we do this in the constructor itself and create a new animator in nextRun?
It seems weird to keep calling initialize on the same object.

> PerformanceTests/SVG/resources/RenderAnimator.js:181
> +        setTimeout(bind(this.startRun, this), this.timeoutDelay);

Why don't we just do: this.startRun.bind(this) instead?

> PerformanceTests/SVG/resources/RenderAnimator.js:184
> +    startRun : function() {

Since this function is only called inside nextRun, I would make this a closure inside nextRun instead.
We have too many functions that start or move to next frame, rect, etc... that it's getting confusing.

> PerformanceTests/SVG/resources/RenderAnimator.js:189
> +    nextTarget : function() {

nextTarget seems like a function that returns the next target rect.
We should verb'fy this function; e.g. moveToNextTargetRectOrFinish.

> PerformanceTests/SVG/resources/RenderAnimator.js:198
> +    nextMove : function() {

I would call this moveToNextAnimateRect.

> PerformanceTests/SVG/resources/RenderAnimator.js:222
> +    maximizeWindow : function() {
> +        window.moveTo(0, 0);
> +        window.resizeTo(screen.width, screen.height);
> +    },

I don't think we want to do this because it would mean that the benchmark result will depend on the screen size.
r- because of this screen-size dependency.

> PerformanceTests/SVG/resources/RenderAnimator.js:240
> +    renderAnimator.initialize();
> +    renderAnimator.nextRun();

We don't we make initialize call nextRun?  Maybe we can rename initialize to "run" if we did that.
Comment 23 Radar WebKit Bug Importer 2015-01-27 15:02:11 PST
<rdar://problem/19621168>
Comment 24 Said Abou-Hallawa 2015-01-27 20:44:58 PST
Created attachment 245513 [details]
Patch
Comment 25 Said Abou-Hallawa 2015-01-27 21:07:48 PST
(In reply to comment #22)
> Comment on attachment 245464 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245464&action=review
> 
> > PerformanceTests/SVG/resources/RenderAnimator.js:3
> > +//
> > +// This code is used to run an SVG benchmark through the WebKit performance test infrastructure.
> > +//
> 
> This seems like an obvious point that doesn't deserve a comment.
> 

The comment was removed

> > PerformanceTests/SVG/resources/RenderAnimator.js:5
> > +//   1. Include it at the end of the body tag in the HTML
> 
> Is this really a requirement?  Can't we just wait for DOMContentLoaded for
> example?
> 

Fixed. Also the script was moved in the <head> section of the HTML.

> > PerformanceTests/SVG/resources/RenderAnimator.js:6
> > +//   2. The HTML has only two elements whose IDs are "Image" and "Timer".
> 
> Can we just insert "Timer" element right after the image element
> automatically instead?
> 

Done. And the script can be used for any element and not necessarily to be an image. The only requirement now is to have an HTML element with className = "Benchmark"

> > PerformanceTests/SVG/resources/RenderAnimator.js:17
> > +function bind(f, obj) {
> > +    return function() {
> > +        return f.apply(obj, arguments);
> > +    };
> > +}
> 
> JavaScript has builtin "bind".  e.g. f.bind(obj).apply(arguments).
> 

Fixed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:69
> > +function ElapsedTimer() {
> 
> I wouldn't call this a timer since it wouldn't schedule anything.
> How about ElapsedTime?
> 

Name was changed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:131
> > +    this.initialize = function() {
> > +        _animateMoveIndex = 0;
> > +        this.nextTargetRect();
> > +        _animateRect = _targetRect;
> > +    };
> 
> Why can't we do this in the constructor itself and create a new animator in
> nextRun?
> It seems weird to keep calling initialize on the same object.
> 

Done.

> > PerformanceTests/SVG/resources/RenderAnimator.js:181
> > +        setTimeout(bind(this.startRun, this), this.timeoutDelay);
> 
> Why don't we just do: this.startRun.bind(this) instead?
> 

Fixed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:184
> > +    startRun : function() {
> 
> Since this function is only called inside nextRun, I would make this a
> closure inside nextRun instead.
> We have too many functions that start or move to next frame, rect, etc...
> that it's getting confusing.
> 

Done.

> > PerformanceTests/SVG/resources/RenderAnimator.js:189
> > +    nextTarget : function() {
> 
> nextTarget seems like a function that returns the next target rect.
> We should verb'fy this function; e.g. moveToNextTargetRectOrFinish.
> 

Name was changed to moveToNextTargetRect() since the meaning is "move to the next target rect if possible". Otherwise I have to name nextMove() to moveToNextAnimateRectOrNextAnimateRect(). I do not think we need to name a function with all its possible outcomes or side effects.

> > PerformanceTests/SVG/resources/RenderAnimator.js:198
> > +    nextMove : function() {
> 
> I would call this moveToNextAnimateRect.
> 

Name was changed as suggested.

> > PerformanceTests/SVG/resources/RenderAnimator.js:222
> > +    maximizeWindow : function() {
> > +        window.moveTo(0, 0);
> > +        window.resizeTo(screen.width, screen.height);
> > +    },
> 
> I don't think we want to do this because it would mean that the benchmark
> result will depend on the screen size.
> r- because of this screen-size dependency.
> 

Done. Now the windowSize is set to (3000, 1500) since I noticed the tiling causes the SVG rendering to slow down significantly when the width is slightly bigger than 2000. If the width is less than 2000, the SVG rendering is very fast even without this patch.

With the windowSize hardcoded and without this patch, the rendering is slow regardless of the browser window size. With this patch, the performance is always faster than without it.

When resizing the window is removed, run-perf-tests is taking few seconds with one target rect. So I added few more target rects to ensure the results are more accurate. On my machine, each test is taking around 40 seconds.

> > PerformanceTests/SVG/resources/RenderAnimator.js:240
> > +    renderAnimator.initialize();
> > +    renderAnimator.nextRun();
> 
> We don't we make initialize call nextRun?  Maybe we can rename initialize to
> "run" if we did that.

renderAnimator.initialize() was removed. It functionality was moved to the object constructor.
Comment 26 Simon Fraser (smfr) 2015-01-28 15:14:03 PST
Comment on attachment 245513 [details]
Patch

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

> Source/WebCore/rendering/RenderImage.cpp:477
> -        bool clip = !contentRect.contains(paintRect);
> -        GraphicsContextStateSaver stateSaver(*context, clip);
> -        if (clip)
> -            context->clip(contentRect);
> +        GraphicsContextStateSaver stateSaver(*context, true);
> +        context->clip(snapRectToDevicePixels(contentRect, deviceScaleFactor));

We think this is wrong.
Comment 27 Said Abou-Hallawa 2015-01-28 17:52:32 PST
Created attachment 245589 [details]
Patch
Comment 28 Said Abou-Hallawa 2015-01-28 17:54:55 PST
(In reply to comment #26)
> Comment on attachment 245513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245513&action=review
> 
> > Source/WebCore/rendering/RenderImage.cpp:477
> > -        bool clip = !contentRect.contains(paintRect);
> > -        GraphicsContextStateSaver stateSaver(*context, clip);
> > -        if (clip)
> > -            context->clip(contentRect);
> > +        GraphicsContextStateSaver stateSaver(*context, true);
> > +        context->clip(snapRectToDevicePixels(contentRect, deviceScaleFactor));
> 
> We think this is wrong.

Yes you are right. I misunderstood the purpose of this code and I thought it is clipping to the actual dirtyRect. The code has changed accordingly.
Comment 29 Ryosuke Niwa 2015-01-28 18:00:00 PST
Comment on attachment 245589 [details]
Patch

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

r=me on the perf. test you're adding. smfr should review the actual code change.

> PerformanceTests/SVG/resources/RenderAnimator.js:4
> +// equal to "Benchmark".  The function runTest() has to be called after the DOMContentLoaded
> +// event is fired.

Instead of commenting this, why don't we just automatically call runTest at DOMContentLoaded?
window.addEventListener('DOMContentLoaded', runTest);
Comment 30 Said Abou-Hallawa 2015-01-28 18:57:52 PST
Created attachment 245597 [details]
Patch
Comment 31 Said Abou-Hallawa 2015-01-28 19:15:12 PST
(In reply to comment #29)
> Comment on attachment 245589 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245589&action=review
> 
> r=me on the perf. test you're adding. smfr should review the actual code
> change.
> 
> > PerformanceTests/SVG/resources/RenderAnimator.js:4
> > +// equal to "Benchmark".  The function runTest() has to be called after the DOMContentLoaded
> > +// event is fired.
> 
> Instead of commenting this, why don't we just automatically call runTest at
> DOMContentLoaded?
> window.addEventListener('DOMContentLoaded', runTest);

I could not use 'DOMContentLoaded' event because it gets fired when the DOM is completely loaded, but without waiting for resources like images to finish loading. In these test, the SVG is not loaded when 'DOMContentLoaded' is fired so the test was getting the wrong image size. So I had to add a listener for 'load' event instead.
Comment 32 WebKit Commit Bot 2015-01-28 21:23:28 PST
Comment on attachment 245597 [details]
Patch

Clearing flags on attachment: 245597

Committed r179335: <http://trac.webkit.org/changeset/179335>
Comment 33 WebKit Commit Bot 2015-01-28 21:23:34 PST
All reviewed patches have been landed.  Closing bug.