Bug 65377 – Hidden text nodes slow down position traversing

• Component: Text
• Status: RESOLVED LATER

[Yong Li]

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.

[Yong Li]

Created attachment 102364 [details]
proposed patch

There may be more places we can optimize in Position related code. Is it a correct approach?

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 102364 [details]
proposed patch

Attachment 102364 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9266519

[Antonio Gomes]

Yong, would you have numbers to share?

[Yong Li]

Created attachment 102367 [details]
proposed patch again

[Yong Li]

(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

[Yong Li]

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

[Darin Adler]

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.

[Yong Li]

Comment on attachment 102367 [details]
proposed patch again

will write a layout test...

[Yong Li]

Created attachment 105019 [details]
With a test case

[Yong Li]

Comment on attachment 105019 [details]
With a test case

forgot to add change log for layouttest

[Yong Li]

Created attachment 105021 [details]
The patch with test case

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

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

[Gyuyoung Kim]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Yong Li]

Created attachment 105035 [details]
the patch

[Darin Adler]

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  <[e-mail]>

Patch deletes the "2" here!

> Source/WebCore/ChangeLog:25
> +011-08-24  Sam Weinig  <[e-mail]>

Patch deletes the "2" here!

[Darin Adler]

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?

[Yong Li]

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

[Eric Seidel (no email)]

Attachment 105035 [details] was posted by a committer and has review+, assigning to Yong Li for commit.

[Yong Li]

Created attachment 123945 [details]
Fix the change log

[WebKit Review Bot]

Comment on attachment 123945 [details]
Fix the change log

Clearing flags on attachment: 123945

Committed r105885: <http://trac.webkit.org/changeset/105885>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

the patch seems obsolete and causes assertions. push this task to later