1371351 - Huge performance regression on document written in Japanese

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Alice0775 White]

Attachment: reduced html (from the 1st link in STR)

Reproducible: always

Steps To Reproduce:
1. Open some html document written in Japanese site(www.raitonoveru.jp/novel/tyou.html)
   e.g. http://www.raitonoveru.jp/novel/okigaru3/08.html
        http://www.raitonoveru.jp/novel/okigaru2/60.html
2. Observe how long it takes displaying the page.


Actual Results:
It takes a very long time(>60s).

Expected Results:
They should be within 5 seconds(Firefox51).


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e48bc2dc52895542e7988b0eafdda2535f68e639&tochange=be7d9851343b6612a322dfa15b11ec099cd94c18

Regressed by: Bug 1081858

Comment 1

[David Baron :dbaron:]

Profile (Linux, today's nightly): https://perfht.ml/2r0rHed

Comment 2

[David Baron :dbaron:]

This shows that we're spending nearly all the time in BuildTextRuns.  (Is something making us repeatedly invalidate and rebuild them?)

Comment 3

[Alice0775 White]

Gecko profiler log (windows10 Nightly55 without e10s using attached html)
https://perfht.ml/2s8j1qu

Comment 4

[Alice0775 White]

Via local build.
Last Good: e48bc2dc5289
First Bad: 8d6560b363d2

Regressed by: 
	8d6560b363d2	Kan-Ru Chen — Bug 1081858 - Part 1. Fix aText off-by-one indexing. r=jfkthame

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Part 1 looks pretty innocent...

Comment 6

[Jonathan Kew [:jfkthame]]

In addition to the perf regression here, on scrolling through one of the pages from comment 0 (http://www.raitonoveru.jp/novel/okigaru3/08.html) I notice that substantial chunks of the text are missing -- there's blank space where text should be. This looks very much like the same issue as bug 1369985, where we're failing to create textruns for some of the frames.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> Part 1 looks pretty innocent...

I think it is "guilty" in that it may have exposed a pre-existing flaw elsewhere, even though (AFAICS) it is correct in itself. The fix there presumably results in whitespace characters being dropped from the text in places where previously they were kept, and this is affecting textrun creation.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Part 1 should not change any behavior though.(In reply to Jonathan Kew (:jfkthame) from comment #7)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> > Part 1 looks pretty innocent...
> 
> I think it is "guilty" in that it may have exposed a pre-existing flaw
> elsewhere, even though (AFAICS) it is correct in itself. The fix there
> presumably results in whitespace characters being dropped from the text in
> places where previously they were kept, and this is affecting textrun
> creation.

Yes. The Part 1. change may look innocent because the issue it fixed is a subtle off-by-one error. The fix enabled a behavior (skip whitespaces) that were never run before. I'm not surprised that it also exposed a corner case that was not an issue before.

Comment 9

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: Massive rendering performance regressions are worth tracking.

Comment 10

[Julien Cristau [:jcristau] (away, back August 16)]

tracking per Nathan.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

I have a patch for bug 1369985 which should also fix this.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

This may not be completely fixed. There is another case which has been identified in bug 1373586 which I found that text run may fail to build, which seems to affect the performance in similar way.

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Xidorn, Kan-ru, is there anything else we should do here? 
How bad is the perf regression now, in Nightly?  

I don't think we need to block 55 release on this, so I'll mark it fix-optional. But we could uplift the work in bug 1369985.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

There is at least another similar case has been identified in bug 1373586 comment 9, which can lead to this issue on some websites as well. We may want to leave it open for now.

I agree that we don't need to block 55 release on this, given it has been regressed since 52.

Comment 15

[Astley Chen (inactive)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> I agree that we don't need to block 55 release on this, given it has been
> regressed since 52.

As the perf issue no longer exists on test sites in comment 0 after bug 1369985 and similar cases would be tracked in bug 1373586. Let's close this bug now.