Closed Bug 1371351 Opened 5 years ago Closed 5 years ago

Huge performance regression on document written in Japanese

Categories

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

52 Branch
x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: alice0775, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: jp-critical, perf, regression)

Attachments

(1 file)

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
Flags: needinfo?(kchen)
Profile (Linux, today's nightly): https://perfht.ml/2r0rHed
This shows that we're spending nearly all the time in BuildTextRuns.  (Is something making us repeatedly invalidate and rebuild them?)
Gecko profiler log (windows10 Nightly55 without e10s using attached html)
https://perfht.ml/2s8j1qu
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
Part 1 looks pretty innocent...
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.
(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.
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.
Flags: needinfo?(kchen)
Blocks: FastReflows
See Also: → 1369985
See Also: → 1373586
[Tracking Requested - why for this release]: Massive rendering performance regressions are worth tracking.
I have a patch for bug 1369985 which should also fix this.
Depends on: 1369985
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.
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(kchen)
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(kchen)
Priority: -- → P3
(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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.