Bug 119230

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.

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.

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

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.

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

Created attachment 216781 [details] Patch

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.

Created attachment 217038 [details] Patch

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.

Created attachment 217197 [details] Patch Fixed comments in previous review.

Comment on attachment 217197 [details] Patch Clearing flags on attachment: 217197 Committed r159488: <http://trac.webkit.org/changeset/159488>

All reviewed patches have been landed. Closing bug.