Closed Bug 1330236 Opened 7 years ago Closed 7 years ago

SVGTextContentElement::GetNumberOfChars is super inefficient due to sync reflows

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

patch v0.1
4.16 KB, patch
ehsan.akhgari
: feedback+
Details | Diff | Splinter Review
Bug 1330236 - Compute SVG getNumberOfChars() quicker in simple cases.
59 bytes, text/x-review-board-request
longsonr
: review+
Details 
See this profile obtained from bug 1326346: <https://clptr.io/2jC9O5H>

Here we're spending 405ms running this function out of about 9 seconds.  It seems that all of this time is going into the reflow triggered by the call to GetPrimaryFrame() there.

jwatt: what can be done about this?
Flags: needinfo?(jwatt)
Based on profiling loading this in Chrome, the calls to getNumberOfChars() there take 80ms over all.
(In reply to :Ehsan Akhgari (in Taipei, laggy response time) from comment #0)
> It
> seems that all of this time is going into the reflow triggered by the call
> to GetPrimaryFrame() there.

Sorry that's not entirely correct, this is where we flush <http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/dom/base/Element.cpp#2181>.  Chrome also flushes layout, FWIW.
In general, we must flush layout, since getNumberOfChars() returns the number of characters "addressable" by SVG text positioning attributes (x="", y="", etc. on <text> and <tspan>), which takes into account white space collapsing, trimming of spaces at the end of lines, and perhaps some other cases, all of which are determined while reflowing text frames.

Can you give some more details on how getNumberOfChars() is being used?  Maybe we can avoid doing some work in some simpler cases.
(In reply to Cameron McCormack (:heycam) from comment #3)
> Can you give some more details on how getNumberOfChars() is being used? 
> Maybe we can avoid doing some work in some simpler cases.

Beyond the profile in comment 0?  I don't really know much about their code...  I only noticed the function being called tons of time in the test case.  Try searching for "getnumberofchars" in the profile call tree view to find them all.  The callers are of course all minified code.
Looks like there are a lot of getNumberOfChars() calls, and each one is called on a <text> element whose contents are a bit of text from the slide, e.g.:

$14 = (nsAString_internal::char_type *) 0x7fe1c645fa08 u"<text style=\"font-family:Arial;font-size:37.", '3' <repeats 14 times>, "px;fill:#000000;\" y=\"0px\" text-rendering=\"geometricPrecision\">S</text>"
$15 = (nsAString_internal::char_type *) 0x7fe1c6460808 u"<text style=\"font-family:Arial;font-size:37.", '3' <repeats 14 times>, "px;fill:#000000;\" y=\"0px\" text-rendering=\"geometricPrecision\">I</text>"
$16 = (nsAString_internal::char_type *) 0x7fe1c6461408 u"<text style=\"font-family:Arial;font-size:37.", '3' <repeats 14 times>, "px;fill:#000000;\" y=\"0px\" text-rendering=\"geometricPrecision\">D</text>"
$17 = (nsAString_internal::char_type *) 0x7fe1c73d2808 u"<text style=\"font-family:Arial;font-size:37.", '3' <repeats 14 times>, "px;fill:#000000;\" y=\"0px\" text-rendering=\"geometricPrecision\">¶</text>"
$18 = (nsAString_internal::char_type *) 0x7fe1c653aa08 u"<text style=\"font-family:Arial;font-size:14.", '6' <repeats 15 times>, "px;fill:#FFFFFF;font-weight:700;font-style:italic;\" y=\"0px\" text-rendering=\"geometricPrecision\">Initiative</text>"
$19 = (nsAString_internal::char_type *) 0x7fe1c653aa08 u"<text style=\"font-family:Arial;font-size:14.", '6' <repeats 15 times>, "px;fill:#FFFFFF;font-weight:700;font-style:italic;\" y=\"0px\" text-rendering=\"geometricPrecision\">I</text>"

Each one is done after appending the <text> to an <svg> (that is a child of the <body>, but hidden off the side of the page).  So at the time getNumberOfChars() is called, the <text> doesn't have a frame.  Then the <text> is removed again.

Why it's calling getNumberOfChars() I'm not sure, since it doesn't seem to be calling any other SVG DOM text methods that take addressable character indexes.

Luckily, these are all quite simple cases.  None seem to have any white space characters in them.  So if we want, we could check that none of the descendant text nodes of the <text> include any characters that could be marked as "skip" by nsTextFrameUtils::TransformText (space, tab, newline, SHY, bidi control characters), which is how (along with end-of-line trimmed white space) are the characters excluded when iterating to find the addressable characters:

http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/layout/svg/SVGTextFrame.cpp#2696-2698

If we didn't have to continue returning 0 if the <text> is display:none, then we could avoid flushing style too.
Attached patch patch v0.1 — — Splinter Review 
Ehsan, can you try this patch and see if it helps?  I've verified that we take this faster path that only flushes style and not layout with all of the getNumberOfChars() calls that the Google Slides document does during page load.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(jwatt)
Attachment #8826124 - Flags: feedback?(ehsan)
Thanks Cameron, this patch cuts down the reflow factor by a factor of 1/4th, bringing it down to about 100ms, so it makes us ~300ms faster in this test case.
Attachment #8826124 - Flags: feedback?(ehsan) → feedback+
Attachment #8826530 - Flags: review?(longsonr)
Comment on attachment 8826530 [details]
Bug 1330236 - Compute SVG getNumberOfChars() quicker in simple cases.

https://reviewboard.mozilla.org/r/104482/#review105374

r=longsonr with comments addressed.

::: dom/svg/SVGTextContentElement.cpp:54
(Diff revision 1)
>  
> +SVGTextFrame*
> +SVGTextContentElement::GetSVGTextFrameForNonLayoutDependentQuery()
> +{
> +  nsIFrame* frame = GetPrimaryFrame(FlushType::Frames);
> +  while (frame) {

This loop is equivalent to nsLayoutUtils::GetClosestFrameOfType(frame, svgTextFrame) isn't it?
Attachment #8826530 - Flags: review?(longsonr) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32a19e85b730
Compute SVG getNumberOfChars() quicker in simple cases. r=longsonr+218550
Shako, can you please see if the landing of this bug would move the needle on the Hasal test for bug 1326346?  (Note that this hasn't yet merged to mozilla-central...)  Thanks!
Flags: needinfo?(sho)
https://hg.mozilla.org/mozilla-central/rev/32a19e85b730
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Sure, the test is triggered, will update the test result after the test finish.
Flags: needinfo?(sho)
Hi Ehsan,

  The test result as below:

  Test script name : test_firefox_gslide_read_table_1

                              MEDIAN         	AVG            	STD            	SI             	PSI  
Result with patch:            10740.0        	10673.4013605  	389.23920901   	2368.5         	1631.0
Result without patch:         10330.0        	10346.7482993  	515.026296848  	2565.0         	1613.5
Do we know why this increased the median runtime for the test? Maybe because the test is measuring end-to-end time and not *just* the slide loading?
Flags: needinfo?(ehsan)
I have no idea, but the change is below the standard deviation, so I don't think it necessarily means anything.

Shako, how reliable are these numbers?  Do you consistently detect a regression with this patch compared to without?  Can you please go to about:buildconfig in both of the builds being tested and paste the hg links here?
Flags: needinfo?(ehsan) → needinfo?(sho)
Hi Ehsan,

Both of the results are generated based on 30 times test against real network. And I only trigger one round(30 times tests) for each build. 

hg link without patch: https://hg.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571

hg link with patch: https://hg.mozilla.org/mozilla-central/rev/80eac484366ad881c6a10bf81e8d9b8f7a676c75
Flags: needinfo?(sho)
It's probably worth trying two builds with *only* this patch as the difference. I'll try to find time to push try builds at some point ...
Flags: needinfo?(overholt)
Shako, can you try comparing https://hg.mozilla.org/mozilla-central/rev/09fb98312335 against https://hg.mozilla.org/mozilla-central/rev/32a19e85b730 (that should be *just* the change from this bug)? Thanks!
Flags: needinfo?(overholt) → needinfo?(sho)
Hi Andrew,

  I try to download the build from the treeherder default view of this two changeset, but they link to the same build revision, is that correct? Or I should use another link to download the build?

  BTW, I still trigger the test, the result below is for revision: 80eac484366ad881c6a10bf81e8d9b8f7a676c75

                              MEDIAN         	AVG            	STD            	SI             	PSI  
gslide_read_table_1:          8200.0        	8203.33333333  	197.860781584   1594.5          817.0
Flags: needinfo?(sho)
You're right, I forgot they'd have ended up in the same push. I'll push some builds to try myself and get you the links.
Flags: needinfo?(overholt)
I'm clearly never going to get to this.
Flags: needinfo?(overholt)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: