Created attachment 102361 [details] the test case When WebKit traverses positions, it wastes CPU time on hidden text nodes. Open the test case in a WebKit browser, click the blank area on the left of the visible text, you will see the page doesn't respond for text selection any more. This is because WebKit is busy in traversing hidden text nodes. The test case doesn't exist in real world. However pages with hidden text do exist in real world, and if we fix this, we could probably boost selection performance in some cases.
Created attachment 102364 [details] proposed patch There may be more places we can optimize in Position related code. Is it a correct approach?
Attachment 102364 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/editing/htmlediting.cpp:222: Declaration has space between type name and * in RenderObject *renderer [whitespace/declaration] [3] Source/WebCore/editing/htmlediting.cpp:248: Declaration has space between type name and * in RenderObject *renderer [whitespace/declaration] [3] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 102364 [details] proposed patch Attachment 102364 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9266519
Yong, would you have numbers to share?
Created attachment 102367 [details] proposed patch again
(In reply to comment #4) > Yong, would you have numbers to share? No numbers. The result is significant with the test case. Without the patch, browser freezes. With the patch, you cannot feel any slowness
(In reply to comment #0) > Created an attachment (id=102361) [details] > the test case > > When WebKit traverses positions, it wastes CPU time on hidden text nodes. > > Open the test case in a WebKit browser, click the blank area on the left of the visible text, you will see the page doesn't respond for text selection any more. This is because WebKit is busy in traversing hidden text nodes. > > The test case doesn't exist in real world. However pages with hidden text do exist in real world, and if we fix this, we could probably boost selection performance in some cases. Correction: "click the blank area on the left of..." shouldbe "on the right of"
Comment on attachment 102367 [details] proposed patch again View in context: https://bugs.webkit.org/attachment.cgi?id=102367&action=review > Source/WebCore/ChangeLog:10 > + No new tests because it doesn't fix any layout issue. We have a layout test system for making performance tests that check that performance has the correct order of magnitude. These tests are in the LayoutTests/perf directory using the Magnitude object. Using that you should be able to make a test that uses giant hidden text nodes to show the massive speedup from this change.
Comment on attachment 102367 [details] proposed patch again will write a layout test...
Created attachment 105019 [details] With a test case
Comment on attachment 105019 [details] With a test case forgot to add change log for layouttest
Created attachment 105021 [details] The patch with test case
Comment on attachment 105021 [details] The patch with test case Attachment 105021 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9486175
Comment on attachment 105021 [details] The patch with test case Attachment 105021 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9487204
Comment on attachment 105021 [details] The patch with test case Attachment 105021 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9490139
Comment on attachment 105021 [details] The patch with test case Attachment 105021 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9488190
Comment on attachment 105021 [details] The patch with test case Attachment 105021 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9486194
Created attachment 105035 [details] the patch
Comment on attachment 105035 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=105035&action=review > LayoutTests/ChangeLog:13 > +011-08-24 Andrew Scherkus <scherkus@chromium.org> Patch deletes the "2" here! > Source/WebCore/ChangeLog:25 > +011-08-24 Sam Weinig <sam@webkit.org> Patch deletes the "2" here!
Comment on attachment 105035 [details] the patch There are 8 different code changes here. There is only one test case. Does the test cover all 8 changes? Or do we need to make more test cases to make sure we cover the improvements in all 8?
(In reply to comment #20) > (From update of attachment 105035 [details]) > There are 8 different code changes here. There is only one test case. Does the test cover all 8 changes? Or do we need to make more test cases to make sure we cover the improvements in all 8? Actually 6 of them can be tested by just manually clicking on the blank area of the page. However I cannot make DRT show the problem by using eventSender to simulate mouse events. So I ended up with using selection, which tests the other 2 changes. I'll give a further try...
Attachment 105035 [details] was posted by a committer and has review+, assigning to Yong Li for commit.
Created attachment 123945 [details] Fix the change log
Comment on attachment 123945 [details] Fix the change log Clearing flags on attachment: 123945 Committed r105885: <http://trac.webkit.org/changeset/105885>
All reviewed patches have been landed. Closing bug.
the patch seems obsolete and causes assertions. push this task to later