Bug 119230 – [CSS Regions] Add performance test for selection

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

[Manuel Rego Casasnovas]

Investigate performance issues in selection for documents using CSS Regions.

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

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.

[Zoltan Horvath]

(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.

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 216781 [details]
Patch

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 217038 [details]
Patch

[Ryosuke Niwa]

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.

[Manuel Rego Casasnovas]

Created attachment 217197 [details]
Patch

Fixed comments in previous review.

[WebKit Commit Bot]

Comment on attachment 217197 [details]
Patch

Clearing flags on attachment: 217197

Committed r159488: <http://trac.webkit.org/changeset/159488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.