UNCONFIRMED 120512
PositionIterator increment and decrement functions can be sped up by not iterating through nodes without renderers 
https://bugs.webkit.org/show_bug.cgi?id=120512
Summary PositionIterator increment and decrement functions can be sped up by not iter...
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
DetailsFormatted DiffDiff
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-
DetailsFormatted DiffDiff
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"> <!--Some comment to iterate through--> <div>First</div> <!--Some comment to iterate through--> <div>Next</div> <!--Some comment to iterate through--> </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.