Bug 120512

Summary:

PositionIterator increment and decrement functions can be sped up by not iterating through nodes without renderers

Product:

WebKit

Reporter:

Vani Hegde <vani.hegde>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

bfulgham, buildbot, commit-queue, darin, esprehn+autocc, kangil.han, rniwa, vanivhegde

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Patch with performance test	rniwa: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2	none

Vani Hegde

Reported 2013-08-29 23:24:20 PDT

PositionIterator increment and decrement functions need not iterate through nodes without renderers, as these nodes are neither considered as candidates nor taken into account by upstream/downstream functions. Skipping such nodes would be more efficient.

Attachments
Patch (3.38 KB, patch) 2013-08-29 23:40 PDT, Vani Hegde	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (493.91 KB, application/zip) 2013-08-30 01:50 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (509.33 KB, application/zip) 2013-08-30 07:09 PDT, Build Bot	no flags	Details
Patch with performance test (5.16 KB, patch) 2013-09-06 01:19 PDT, Vani Hegde	rniwa: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (1.58 MB, application/zip) 2013-09-06 03:20 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (454.08 KB, application/zip) 2013-09-07 09:12 PDT, Build Bot	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Vani Hegde

Comment 1 2013-08-29 23:40:52 PDT

Created attachment 210070 [details] Patch

Vani Hegde

Comment 2 2013-08-29 23:42:37 PDT

There is one test case failing with this patch, editing/inserting/return-key-in-hidden-field.html. In the test output, one new line is removed from div #d2, with this change. I happened to come across https://bugs.webkit.org/show_bug.cgi?id=62734, which introduced this additional new line in the test output. Surprisingly this is the only failure and I am not sure which one's the right behavior.

Build Bot

Comment 3 2013-08-30 01:50:36 PDT

Comment on attachment 210070 [details] Patch Attachment 210070 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1624877 New failing tests: editing/inserting/return-key-in-hidden-field.html fast/replaced/width-and-height-of-positioned-replaced-elements.html

Build Bot

Comment 4 2013-08-30 01:50:37 PDT

Created attachment 210077 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Build Bot

Comment 5 2013-08-30 07:09:44 PDT

Comment on attachment 210070 [details] Patch Attachment 210070 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1654074 New failing tests: editing/inserting/return-key-in-hidden-field.html

Build Bot

Comment 6 2013-08-30 07:09:47 PDT

Created attachment 210104 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Darin Adler

Comment 7 2013-08-30 07:58:17 PDT

Comment on attachment 210070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210070&action=review > Source/WebCore/ChangeLog:21 > + No new tests as this is a code refactoring. This is not a code refactoring. It’s a performance fix. We should be able to create a performance test showing the increased speed.

Vani Hegde

Comment 8 2013-09-02 04:59:40 PDT

(In reply to comment #7) > (From update of attachment 210070 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210070&action=review > > > Source/WebCore/ChangeLog:21 > > + No new tests as this is a code refactoring. > > This is not a code refactoring. It’s a performance fix. We should be able to create a performance test showing the increased speed. Thanks for the review Darin. I spent sometime trying to write a performance test and I am stuck with the same. I wrote a simple test <!DOCTYPE html> <html> <head> <script src="../resources/runner.js"></script> <script> function runTest() { function start() { document.execCommand("SelectAll"); } function end() { document.execCommand("Unselect"); } PerfTestRunner.measureRunsPerSecond({ run: start, done: end }); } </script> </head> <body onload='runTest()'> <div id="test">  <div>First</div>  <div>Next</div>  </div> </body> </html> When I directly run this HTML through browser(Ex: QtTestBrowser), it works fine. But when I tried the same through script Tools/Scripts/run-perf-tests --output-json-path=/home/vani.hegde/Desktop/perf_test_result/result.json PerformanceTests/Editing/position-iterator-performance-test.html this fails and I get an error as below Running 1 tests Running Editing/position-iterator-performance-test.html (1 of 1) ERROR: First FAILED Finished: 6.329886 s And I could not resolve this. Not sure if I am missing something here. I would really appreciate some help on this. Thanks!

Darin Adler

Comment 9 2013-09-03 10:01:54 PDT

The style of performance test I had in mind was like the tests in LayoutTests/perf. Look, for example, at the test named LayoutTests/perf/mouse-event.html. The idea is that there is a setup function that takes an integer for magnitude and it scales according to that. So that would be the number of nodes you add, taking care to add a type of node that won’t have renderers. Then the test runs and it proves that the speed of the test does not go down proportional to the number of non-renderer nodes. Except I guess that might not work here, because we still skip through those nodes, just faster than before. Ryosuke might have some advice on how to still use the Magnitude class for testing this.

Ryosuke Niwa

Comment 10 2013-09-03 10:47:47 PDT

I don't think Magnitude class well worked in JSC due to the bug 44199. In fact, Mac port skips all tests in LayoutTests/perf per line below: # Still working out flakiness issues with the perf tests. webkit.org/b/44199 perf/

Vani Hegde

Comment 11 2013-09-06 01:19:17 PDT

Created attachment 210710 [details] Patch with performance test

Vani Hegde

Comment 12 2013-09-06 01:28:18 PDT

I have added a performance test. Below are the results for 5 sample runs with and without the patch +-------------------+-------------------+ | With patch | Without | +------------+---------------+---+-------------------+ | Run 1 | 452.82 ± 0.46% | 303.89 ± 0.36% | +------------+-------------------+-------------------+ | Run 2 | 448.83 ± 0.80% | 302.85 ± 0.49% | +------------+-------------------+-------------------+ | Run 3 | 450.88 ± 0.93% | 301.38 ± 0.67% | +------------+-------------------+-------------------+ | Run 4 | 459.71 ± 0.33% | 302.21 ± 0.40% | +------------+-------------------+-------------------+ | Run 5 | 452.65 ± 0.58% | 294.69 ± 1.95% | +------------+-------------------+-------------------+ Unit: runs/s NOTE: I tried reusing 'Interactive/SelectAll.html'. But this testcase fails to record the performance improvement i.e. time taken is almost same with and without the patch. This testcase uses PerfTestRunner.measureValueAsync to measure the performance and also uses SetTimeout for all function calls. I have eliminated all these in the test case I newly created and used PerfTestRunner.measureRunsPerSecond to measure the performance.

Build Bot

Comment 13 2013-09-06 03:20:27 PDT

Comment on attachment 210710 [details] Patch with performance test Attachment 210710 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1709262 New failing tests: editing/inserting/return-key-in-hidden-field.html

Build Bot

Comment 14 2013-09-06 03:20:30 PDT

Created attachment 210716 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Darin Adler

Comment 15 2013-09-06 12:36:06 PDT

If this test doesn’t show the increased performance, was there any testing you personally did that confirmed that this improves performance?

Build Bot

Comment 16 2013-09-07 09:12:25 PDT

Comment on attachment 210710 [details] Patch with performance test Attachment 210710 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1710698 New failing tests: editing/inserting/return-key-in-hidden-field.html

Build Bot

Comment 17 2013-09-07 09:12:27 PDT

Created attachment 210908 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Vani Hegde

Comment 18 2013-09-09 23:39:15 PDT

(In reply to comment #15) > If this test doesn’t show the increased performance, was there any testing you personally did that confirmed that this improves performance? Hi Darin, As I mentioned in my previous comment(Comment 12 - https://bugs.webkit.org/show_bug.cgi?id=120512#c12), I have already added a performance test as part of the patch uploaded and this test captures the performance improvement(we can see increase in number of runs/s with the patch). I have also posted the statistics in https://bugs.webkit.org/show_bug.cgi?id=120512#c12.

Darin Adler

Comment 19 2013-09-10 01:47:14 PDT

Do we know why the editing/inserting/return-key-in-hidden-field.html test is failing? Is that a false positive, or does this patch break that test?

Vani Hegde

Comment 20 2013-09-10 01:56:13 PDT

(In reply to comment #19) > Do we know why the editing/inserting/return-key-in-hidden-field.html test is failing? Is that a false positive, or does this patch break that test? Well, I am not sure about that. This patch knocks off a new line from the div. I have done some analysis on the history of this test and the details are in Comment 2. However I could not really figure out how/why this patch is knocking off the new line from div.

Darin Adler

Comment 21 2013-09-10 12:23:54 PDT

We have to take some kind of action about the failing test; can’t just check in something that makes one of our regression tests start failing. We can stop running the test, or we can figure out why the change affects the test result and correct the mistake, or we can simply update the test to expect the new result without deep research into why the result is different, assuming that whatever the reason is, it’s not important.

Vani Hegde

Comment 22 2013-09-11 21:29:46 PDT

(In reply to comment #21) > We have to take some kind of action about the failing test; can’t just check in something that makes one of our regression tests start failing. We can stop running the test, or we can figure out why the change affects the test result and correct the mistake, or we can simply update the test to expect the new result without deep research into why the result is different, assuming that whatever the reason is, it’s not important. I have analysed the failure and it is a regression and needs to fixed. Working on it.

Brent Fulgham

Comment 23 2013-10-31 14:55:35 PDT

Any action on this? Vani, if you are working on a revised patch should we clear the review flag so that this doesn't continue to show up in our pending review queue?

Ryosuke Niwa

Comment 24 2013-10-31 21:55:42 PDT

Comment on attachment 210710 [details] Patch with performance test r- due to the failing test.

Vani Hegde

Comment 25 2013-11-05 19:11:10 PST

(In reply to comment #23) > Any action on this? Vani, if you are working on a revised patch should we clear the review flag so that this doesn't continue to show up in our pending review queue? Sorry for the delayed response. Yes, we can clear the review flag.

Note You need to log in before you can comment on or make changes to this bug.