983465 - Performance of getBoundingClientRect on ranges containing whitespace nodes with suppressed frames is poor

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Joseph Jones]

Attachment: boundingrect_text.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140306171728

Steps to reproduce:

1) Unzip attached HTML file
2) Edit file to uncomment the style tag and the correct filter_param code block for the browser you want to test with.
3) Open the browser and make sure that the dev tools console is showing.
4) Load up the file in the browser.
5) 5s after opening a script will run to walk the DOM for a certain set of nodes and get the bounding rects for the nodes. It will report the total time this took when completed.


Actual results:

Time to complete DOM walk:

For FireFox
  Without Multicolumn: ~200ms 
  With Multicolumn: ~30s

For Chrome
  Without Multicolumn: ~30ms
  With Multicolumn: ~30ms

For IE
  Without Multicolumn: ~130ms 
  With Multicolumn: ~130ms



Expected results:

Time for walking the DOM and getting bounding rects should be approximately equivalent. I'd be OK with an order of magnitude but 2+ orders of magnitude is just horrible (might I add embarrassing)?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Based on profile this is all layout/reflow.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(I don't understand why we end up reflowing so many times.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is pretty ridiculous.

What happens here is that nsRange::GetBoundingClientRect calls CollectClientRects, which calls GetPartialTextRect, which calls GetTextFrameForContent, which calls nsCSSFrameConstructor::EnsureFrameForTextNode.

In cases when the textnode had a frame suppressed (i.e. it was whitespace-only) this causes us to create and insert the new frame and then we have to do layout.  In the columns case layout is a lot slower than in the non-columns case to do height-balancing (which Chrome doesn't do particularly well, by the way).

This issue will only affect people asking for getBoundingClientRect on collapsed textnodes...  If I comment out this line of the testcase:

  bounding_rect = range.getBoundingClientRect();

then I get times like so:

For FireFox
  Without Multicolumn: ~24ms 
  With Multicolumn: ~24ms

For Chrome
  Without Multicolumn: ~21ms
  With Multicolumn: ~21ms

Robert, is there any way we can just avoid creating the frames in this situation?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: simple correctness testcase

Chrome deals with this by just being incorrect. Some collapsed-away text nodes (but not all) do not contribute to the list returned by getClientRects (or, presumably, getBoundingClientRect). See this testcase, in which the first box should always be zero-sized.

Also, there should be three boxes in each list. Chrome returns only 1 in the second case, which is inexplicable.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, even in the Joseph's testcase, Chrome is returning incorrect results for all the collapsed-away text nodes

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(It's returning 0,0,0,0 when it should be returning reasonable x/y values.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  IE11 also seems to return only 1 rect, if I'm testing right...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The easiest way to fix this case is to have a document-wide flag to disable the collapsed-away text node optimization when we discover we need the text frame for any collapsed-away node.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Hmm.  IE11 also seems to return only 1 rect, if I'm testing right...

In which part of the test? What does IE11 do for the second part of the test?

Comment 10

[Boris Zbarsky [:bzbarsky]]

For the first part of the test, IE has rects.length == 0.

For the second part of the test, IE has rects.length == 1, and the rect has x=9, y=271.4, width=0, height=0.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This fix reveals a columns bug with the reporter's testcase. Filed as bug 983581.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8391109 [details] [diff] [review]
fix

>     if (frame && frame->GetType() == nsGkAtoms::textFrame) {
>+      aContent->OwnerDoc()->FlushPendingNotifications(Flush_Layout);
>       return static_cast<nsTextFrame*>(frame);
frame maybe a dead object here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8391109 [details] [diff] [review]
fix

Olli is right.  We should either flush before doing the GetPrimaryFrame() call in EnsureFrameForTextNode or reget the primary frame after flushing.

And we should document the mAlwaysCreateFramesForIgnorableWhitespace member to make it clear that we turn off the optimization globally because if people are poking at whitespace sizes/positions they'll probably keep doing it.  Or something like that.

r=me with that

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Olli is right.  We should either flush before doing the GetPrimaryFrame()
> call in EnsureFrameForTextNode or reget the primary frame after flushing.

Good point. I'll do the latter.

> And we should document the mAlwaysCreateFramesForIgnorableWhitespace member
> to make it clear that we turn off the optimization globally because if
> people are poking at whitespace sizes/positions they'll probably keep doing
> it.  Or something like that.

Sure.

Comment 16

[Joseph Jones]

(In reply to Boris Zbarsky [:bz] from comment #3)
> This is pretty ridiculous.
> 
> What happens here is that nsRange::GetBoundingClientRect calls
> CollectClientRects, which calls GetPartialTextRect, which calls
> GetTextFrameForContent, which calls
> nsCSSFrameConstructor::EnsureFrameForTextNode.
> 
> In cases when the textnode had a frame suppressed (i.e. it was
> whitespace-only) this causes us to create and insert the new frame and then
> we have to do layout.  In the columns case layout is a lot slower than in
> the non-columns case to do height-balancing (which Chrome doesn't do
> particularly well, by the way).
> 
> This issue will only affect people asking for getBoundingClientRect on
> collapsed textnodes...  If I comment out this line of the testcase:
> 
>   bounding_rect = range.getBoundingClientRect();
> 
> then I get times like so:
> 
> For FireFox
>   Without Multicolumn: ~24ms 
>   With Multicolumn: ~24ms
> 
> For Chrome
>   Without Multicolumn: ~21ms
>   With Multicolumn: ~21ms
> 
> Robert, is there any way we can just avoid creating the frames in this
> situation?

Is there a way to tell if a text nod eis collapsed on the client side?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Not easily.  It depends on all sorts of stuff.  A prerequisite is it being whitespace-only (and even that doesn't have a nice API to detect it), but various other conditions apply too.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0af8be2215b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0af8be2215b

Comment 20

[Camelia Badau [:cbadau], Desktop QA]

Verified on Mac OSX 10.8.5 using Firefox 31.0b8 and the following times are achieved: 
         Without Multicolumn: ~88ms 
         With Multicolumn: ~120ms
    
- If I comment out this line of the testcase: bounding_rect = range.getBoundingClientRect(); then I get times like so:
        Without Multicolumn: ~27ms 
        With Multicolumn: ~27ms