Bug 119230 - [CSS Regions] Add performance test for selection
Summary: [CSS Regions] Add performance test for selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 118668 124273 124281
  Show dependency treegraph
 
Reported: 2013-07-29 16:18 PDT by Manuel Rego Casasnovas
Modified: 2013-11-19 01:26 PST (History)
9 users (show)

See Also:


Attachments
Example of performance tests for selection in a document with and without CSS Regions (7.73 KB, patch)
2013-07-29 16:20 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Results of perf-tests for selection in CSS Regions (9.40 KB, application/x-gzip)
2013-07-29 16:29 PDT, Manuel Rego Casasnovas
no flags Details
Example of performance tests for selection in a document with and without CSS Regions (6.84 KB, patch)
2013-10-26 04:25 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
New example of performance tests for selection in a document with and without CSS Regions (4.99 KB, patch)
2013-10-28 04:36 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2013-11-13 01:16 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2013-11-15 02:53 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2013-11-15 03:14 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2013-11-18 07:36 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2013-07-29 16:18:36 PDT
Investigate performance issues in selection for documents using CSS Regions.
Comment 1 Manuel Rego Casasnovas 2013-07-29 16:20:46 PDT
Created attachment 207679 [details]
Example of performance tests for selection in a document with and without CSS Regions

I've been testing the performance of selection in documents with and without CSS Regions with the performance tests provided in the attached patch.
Comment 2 Manuel Rego Casasnovas 2013-07-29 16:29:37 PDT
Created attachment 207680 [details]
Results of perf-tests for selection in CSS Regions

These are the results of running the performance tests in a simple document with and without CSS Regions.

The first 3 runs were executed in trunk and the last 3 were executed in trunk without the patch for bug #105641 (r139197).

In trunk the performance of the selection in documents with CSS Regions seems better than in normal documents, I guess that the reason is that when you're selecting different flow threads the execution enter in the diff at RenderView::setSelection() related to the flow threads (see http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderView.cpp#L728). However, without that if (patch for bug #105641) the performance in documents with CSS Regions seems worse.
Comment 3 Zoltan Horvath 2013-07-30 10:48:07 PDT
(In reply to comment #2)
> In trunk the performance of the selection in documents with CSS Regions seems better than in normal documents, I guess that the reason is that when you're selecting different flow threads the execution enter in the diff at RenderView::setSelection() related to the flow threads (see http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderView.cpp#L728). However, without that if (patch for bug #105641) the performance in documents with CSS Regions seems worse.

Yeah, it's because no real selection is happens in regions case when you select content over different regions, so we can't compare the performance until it's fixed. 
It's good to see perf-tests on this area. I think it would be nice to contribute them to the perftests when bug #106817 gets fixed.
Comment 4 Manuel Rego Casasnovas 2013-10-26 04:25:39 PDT
Created attachment 215245 [details]
Example of performance tests for selection in a document with and without CSS Regions

Now that the changes done in bug #105641 (r139197) are removed in trunk by bug #119622 (r155058) we can see the performance issues in the selection with CSS regions.

The new patch is an example of 2 tests that use bigger HTML files as example (a page with 50 regular divs and 50 regions).

The tests perform several selections from keywords in each <div> and the results shows the performance issues:
* Interactive/SelectNoRegions.html: ~60 ms
* Interactive/SelectRegions.html: ~140 ms

So this should be valid to start taking a look to the different issues and try to provide improvements.

Anyway I've found some performance tests related to regions under "PerformanceTests/Layout/" folder, so I'd reuse some stuff there to prepare new selection tests.
Comment 5 Manuel Rego Casasnovas 2013-10-28 04:36:06 PDT
Created attachment 215295 [details]
New example of performance tests for selection in a document with and without CSS Regions

This patch contains tests following the structure of regions tests under "PerformanceTests/Layout/" folder.

Again the differences between a document with and without regions are quite clear:
* Layout/RegularSelection.html: ~170 ms
* Layout/RegionsSelection.html: ~1051 ms
Comment 6 Manuel Rego Casasnovas 2013-11-13 01:16:00 PST
Created attachment 216781 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2013-11-15 02:53:27 PST
Created attachment 217037 [details]
Patch

Using mouse events to simulate a real selection from the first region to the last one passing through all the regions.
Comment 8 Manuel Rego Casasnovas 2013-11-15 03:14:01 PST
Created attachment 217038 [details]
Patch
Comment 9 Ryosuke Niwa 2013-11-18 07:16:06 PST
Comment on attachment 217038 [details]
Patch

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

> PerformanceTests/Layout/resources/regions.js:103
> +            // Clear selection
> +            eventSender.mouseMoveTo(0, 0);
> +            eventSender.mouseDown();
> +            eventSender.mouseUp();

Please call getSelection().removeAllRanges() instead.
It's kind of strange to clear the selection here because that'll mean that we'll be measuring the time to clear the selection as well
but only in the non-first runs.  I'd rather clear the selection at the end instead so that measurements between runs will be homogenous.
You can explicitly clear the selection in the setup function as well.

> PerformanceTests/Layout/resources/regions.js:105
> +            // Start selection

This comment repeats the code. Please remove it.

> PerformanceTests/Layout/resources/regions.js:112
> +            // Move through all the regions
> +            for (var i = 1; i < regionCount; i++) {
> +                mouseMoveToRegionCenter(regions[i]);
> +            }

Remove the comment & no curly brackets around a single statement.

> PerformanceTests/Layout/resources/regions.js:114
> +            // Finish selection

Ditto.

> PerformanceTests/Layout/resources/regions.js:132
> +            setup: function() {
> +                PerfTestRunner.resetRandomSeed();
> +            },

You shouldn't have to do this but okay.
Comment 10 Manuel Rego Casasnovas 2013-11-18 07:36:08 PST
Created attachment 217197 [details]
Patch

Fixed comments in previous review.
Comment 11 WebKit Commit Bot 2013-11-19 01:26:18 PST
Comment on attachment 217197 [details]
Patch

Clearing flags on attachment: 217197

Committed r159488: <http://trac.webkit.org/changeset/159488>
Comment 12 WebKit Commit Bot 2013-11-19 01:26:22 PST
All reviewed patches have been landed.  Closing bug.