Closed Bug 722641 Opened 13 years ago Closed 3 years ago

Displaying of the input text is laggy/choppy, especially using IME

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Version:
12 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox12 + fixed
firefox13 - ---

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: inputmethod, regression)

Attachments

(2 files)

peste text (Long text 1800 lines)
(deleted), text/plain
Details
Back out bug 598482 because it reduces perceived responsiveness in some rare cases.
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Attached file peste text (Long text 1800 lines) (deleted) — 
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/e33539a90ae2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120130 Firefox/12.0a1 ID:20120130031142

Displaying of the input text is laggy/choppy, especially using IME.


Type "a"  and hold key down, 
It is displayed every three or four characters.
It should be displayed smoothly one by one character.

It become worse using IME.


Reproducible: Always

Steps to Reproduce:
1. Start Nightly with new profile
2. Open Wikipedia editing page ( Ex. http://ja.wikipedia.org/w/index.php?title=Wikipedia:%E3%82%B5%E3%83%B3%E3%83%89%E3%83%9C%E3%83%83%E3%82%AF%E3%82%B9&action=edit )
3. Peste long text into textarea.
4. Scroll to top
5. (IME on if any) Type "a" and old key down

Actual Results:  
  It is displayed every three or four characters.
  Displaying of the input text is laggy/choppy.

Expected Results:  
  It should be displayed smoothly one by one character.
  it should be at usual speed and smooth.

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/49afabda6701
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1 ID:20120115033312
Bad:
http://hg.mozilla.org/mozilla-central/rev/823072af2430
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1 ID:20120115035713
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=49afabda6701&tochange=823072af2430


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/59cb54c6dfe1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120114 Firefox/12.0a1 ID:20120114195007
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5e6e63f3aed8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120114 Firefox/12.0a1 ID:20120114201408
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=59cb54c6dfe1&tochange=5e6e63f3aed8
Sorry,
s/5. (IME on if any) Type "a" and old key down/5. (IME on if any) Type "a" and hold key down
The expensive part here is the reflow which is triggered by the willpaint event. Before bug 598482, a reflow happened immediately after every letter, but now we can have multiple letters in the queue between reflows.
Ideally, this would make things smoother, not choppier. But since that isn't the case, is it possible that the cost of this reflow increases with the number of pending letters? Then we'd have gone from an even work distribution (1,1,1,1,1,1,1,1,1,1) to a very unfortunate, choppy one (1,0,2,0,0,3,0,0,3,0,0,3).
Essentially, we add work to the queue faster than we can process it, which makes the queue grow faster, which makes the processing step take longer, until we reach an equilibrium. In this queue model, the right thing to do would be to limit the amount of work we allow into the queue before the next processing step. Mapping that to the reflow problem seems really hard, since there's probably no easy way of predicting how long the reflow of a given change will take...
Reflow of more than one letter should take about the same amount of time as reflow of one letter, I'd think.

We've had this problem before, I believe.  At the time, editor "solved" it by forcing paint flushes after every edit operation.  Then we moved to forcing layout flushes.  Then we stopped forcing flushes, but apparently we were still doing the layout per-letter?  Why?  What triggered those WillPaints?  I guess it might have been the invalidate from the layout of the previous letter....

In any case, I think comment 2 is right and that each reflow takes longer than the key repeat interval.

Alice0775 White, does the description that follow describe what you see?  Before the changes in bug 598482, we ended up doing one reflow per letter, so the letters appeared at a rate slower than key repeat and once you let go of the key a few more letters would appear from the backlog.  Now we still have chunks appearing at a lower rate than key repeat so that the letters appear a few at a time, but there should be less backlog and we should be averaging closer to key repeat.

If that description is _not_ correct it would be very good to know that.  One way to test whether it is is to time how long it takes for 100 'a' characters appear with the old and new builds.  Could you do that?

(Needless to say, I'm not seeing the problem over here; otherwise I wouldn't be asking questions.)
When IME on,
Before landing  bug 598482,1 1 1 1 1 1 1 1 1 , and in practice,there was no problem.
However after landing  bug 598482,typed letter appears like 1 0 0 3 0 0 0 0 0 6 0 0 0 0 0 0 0 0 0 0 0 12 0 0 0 0...24, it is annoying,
CCing:masayuki san
Alice, that sounds really bad.  Can you do the timing test I suggested in comment 3?

I really need a way to reproduce.  Any idea how to turn on the IME on Mac?
(In reply to Boris Zbarsky (:bz) from comment #6)
> Can you do the timing test I suggested in comment 3?

IME off:
   Prior landing  bug 598482 : about 7sec/100characters
   After  landing  bug 598482 : about 10sec/100characters

IME on:
   Prior landing  bug 598482 : about 7sec/100characters.
   After  landing  bug 598482 : about 10sec/100characters
Alright.  That's clearly bad.  That's us doing more work somehow, not just distributing the same work differently....
(In reply to Boris Zbarsky (:bz) from comment #8)
> Alright.  That's clearly bad.  That's us doing more work somehow, not just
> distributing the same work differently....

Sorry,
I have mistake.

IME off:
   After landing  bug 598482 : about 7sec/100characters
   Prior  landing  bug 598482 : about 10sec/100characters

IME on:
   After landing  bug 598482 : about 7sec/100characters
   Prior  landing  bug 598482 : about 10sec/100characters

** Total speed was improved by bug 598482.
I can easily reproduce this using the STR in comment 0 by just pressing 'a' (no IME).

Under a profile, we spend 40% of our time under nsRefreshDriver::Notify.  23% of that is spent under PresShell::ProcessSynthMouseMoveEvent (why???!!!) and 17% of that is under PresShell::FlushPendingNotifications.  Almost all of the reflow time is spent in creating text runs.  The ProcessSynthMouseMoveEvent also results in a reflow.  So, if I'm not mistaken, we reflow twice in each refresh driver tick.  But that shouldn't happen, right? :/
It can happen if either something posted more reflow commands after the refresh driver reflowed or the the mousemove itself leads to reflow bits being set.  That shouldn't _normally_ happen, though.

Where did you have your mouse while you were doing this?
OK, I think I can reproduce the two reflows...

When I look at this more closely, the synth mouse move processing is happening off the refresh driver for the toplevel XUL document.  But this can be out of sync with the refresh driver for the content document, of course.  So we end up with two reflows.

We could try to make SynthesizeMouseMove not walk up to the root presshell until we tick (or at least use the refresh driver of the presshell synthesizing the mouse move).  Would that make sense?  Seems like it would reduce the number of reflows here, but we'd still be subject to being slower than before...  The fundamental problem is that we're doing the work in bigger chunks now, which is desirable in most cases, but means that we're not showing each char separately...
(In reply to Boris Zbarsky (:bz) from comment #11)
> It can happen if either something posted more reflow commands after the
> refresh driver reflowed or the the mousemove itself leads to reflow bits
> being set.  That shouldn't _normally_ happen, though.
> 
> Where did you have your mouse while you were doing this?

The mouse was not moved at all, AFAICT.  That's what was weird!
I'd still like to know where the synth mouse events are coming from.
Synth mouse events are generated any time there's a reflow.  That's what makes them "synth".  ;)
(In reply to Boris Zbarsky (:bz) from comment #12)
> The fundamental problem is that we're doing the work in
> bigger chunks now, which is desirable in most cases, but means that we're
> not showing each char separately...

I'm confused by this. Reflowing after adding multiple characters should definitely be no more expensive than reflowing after adding one character. So we should be repainting at the same rate, just more characters per paint. That wouldn't be a regression IMHO.

We should fix the double-reflow issue.

We should also look into why this is expensive. The textrun and textframe code should be able to append to this textarea incrementally without rebuilding all textruns or reflowing every line.
> So we should be repainting at the same rate, just more characters per paint.

Yes.  The result just _looks_ more jumpy, esp if we're not managing to paint at 60Hz.

> We should fix the double-reflow issue.

Yes.  Want me to spin that off into a separate bug?

> We should also look into why this is expensive.

Note that the steps to reproduce involve prepending, not appending.

The profile I have shows about 61000 samples over 7 seconds sampling every 100us.  So we were using the CPU at least 85% of the time.

Of the time we were using the CPU, 91% is reflow (35% off the refresh driver, 35% off the synth mouse event, 21% off the WillPaint call when the viewmanager goes to paint).  About 57000 samples under ProcessReflowCommands total.

Of the time under ProcessReflowCommands (all percentages now of the 57000 samples):

  53% under EnsureTextRun (32% creating textruns, 13% linebreaker)
   8% under BreakAndMeasureText
  14% under nsBlockFrame::PlaceLine; half of this is nsBidiPresUtils::ReorderFrames
  11% under nsBidiPresUtils::Resolve

I'll bet money the bidi stuff is somehow making us reflow the whole textarea, not the one line we're typing on.
ccing Simon here, given the bidi involvement.
(In reply to Boris Zbarsky (:bz) from comment #17)
> Yes.  Want me to spin that off into a separate bug?

I don't care, here or a new bug is fine.
(In reply to Boris Zbarsky (:bz) from comment #6)
> I really need a way to reproduce.  Any idea how to turn on the IME on Mac?

In System Preferences | Language and Text | Input Sources, check the checkbox next to "Kotoeri" and at least one of "Hiragana" and "Katakana" beneath it. That should give you Hiragana/Katakana entries in the keyboard menu.
Yeah, enabling the IME doesn't seem to change much.

I filed bug 723378 on the synth mouse move issue.
Bug 646359 will help with this if I can fix the remaining issues with it.
Depends on: 646359
It's not clear to me that this needs to be tracked for the FF12 release unless this affects more than prepending the text box by holding down a character. Does quickly typing also reproduce the issue?
(In reply to Alex Keybl [:akeybl] from comment #23)
> It's not clear to me that this needs to be tracked for the FF12 release
> unless this affects more than prepending the text box by holding down a
> character. Does quickly typing also reproduce the issue?

And how brutal is the lag in regular circumstances? If this is an FF12-specific regression and the delay is noticeable, can we back out the changes that caused it while we debug? Do the benefits of that fix outweigh this cost?

Speculatively tracking until we get answers - happy to untrack if this is a very edge-case situation.
tracking-firefox12: ? → +
(In reply to Johnathan Nightingale [:johnath] from comment #24)

> And how brutal is the lag in regular circumstances? 
It is only when edit large amount text with/without IME.
And I think It is not so frequent.
Since this is a suspected regression from bug 598482, assigning to bz for investigation.

I'm also interested in finding out

(In reply to Johnathan Nightingale [:johnath] from comment #24)
> Do the benefits of that fix outweigh
> this cost?
Assignee: nobody → bzbarsky
In case it wasn't clear from bug 598482, I didn't write most of that code.  I just helped drive it in.  So I'm not sure what action is expected from me here other than backing that patch out.

The benefits of that patch are significantly improved performance in many cases (including this one, actually; see comment 9; the problem is that throughput increased at a cost in perceived responsiveness), and much better architecture that we can now build on to do other improvements.  And in fact, we have been doing that.

This bug is, as far as I can tell, limited to large textareas containing text that triggers bidi handling.  This last is not uncommon in some locales, of course.  The right way to fix things here, as far as we can tell, is to fix bug 646359, which Simon is working on.
(In reply to Boris Zbarsky (:bz) from comment #27)
> In case it wasn't clear from bug 598482, I didn't write most of that code. 
> I just helped drive it in.  So I'm not sure what action is expected from me
> here other than backing that patch out.

Thanks for the explanation. Since it's not clear how commonly our users will run into input lag, let's exercise caution and wait a cycle for bug 598482 to land. If you wouldn't mind backing out 598482 from Aurora 12, it'd be much appreciated. Presumably bug 646359 will land on m-c 13 soon since it has a patch r+'d, so no need to back out there yet.
I'll see what I can do, but keep in mind that the backout is pretty risky too, since other things may be depending on the new setup....
Presumably I need to back out bug 718631 on aurora 12 as well?  Michael, can you confirm?
(In reply to Boris Zbarsky (:bz) from comment #30)
> Presumably I need to back out bug 718631 on aurora 12 as well?  Michael, can
> you confirm?

Entirely optional and up to you. There are no developer or build machines building gonk/B2G off release branches.
Timothy, could you give the nsViewManager bits a once-over?
Attachment #601182 - Flags: review?(tnikkel)
Comment on attachment 601182 [details] [diff] [review]
Back out bug 598482 because it reduces perceived responsiveness in some rare cases.

The nsViewManager.cpp changes look like a good backout. Sad that we have to back this out, at least its only on Aurora.
Attachment #601182 - Flags: review?(tnikkel) → review+
Looks like all android tests fail on aurora equally with or without this patch, so I'm going to just push it.
Can somebody take a look at making a forward fix? We'll likely have to back out of Aurora 13 as well if this isn't grabbed by somebody and fixed soon.
I don't think we should keep backing out bug 598482 for this, imo, edge case.  That will involve forgoing various performance wins, especially as people have kept building performance improvements on top of bug 598482...

The fact is, performance on this testcase is terrible.  It used to be terrible in the "I type text, and it shows up one character at a time, with a lot of lag between characters" sense; now it's terrible in the "I type text, and it shows up a few characters at a time with a lot of lag between groups" sense.  We do need to fix the performance, but backing out bug 598482 does NOT do that.  It just takes us back to the old, still broken, behavior.
^^ Agreed. Can we get the fix for 598482 back in?
You mean on beta 12?  I'm not sure I want to rock the boat that much....

The fix for bug 598482 is still present on m-c and aurora 13; I just don't think we should be backing it out from there.  ;)
(In reply to Boris Zbarsky (:bz) from comment #37)
> I don't think we should keep backing out bug 598482 for this, imo, edge
> case.  That will involve forgoing various performance wins, especially as
> people have kept building performance improvements on top of bug 598482...

Do we have STR for the performance issues prior to landing bug 598482 so that QA can verify which is the lesser of two evils input-perf-wise? If it's not significantly worse with bug 598482, the right call would be to leave bug 598482 in Aurora 13 as you suggest given the fact that there have been perf improvements built on top of it.
> Do we have STR for the performance issues prior to landing bug 598482

Just load the testcase in this bug and type.  It takes longer to type the same amount of text before that patch than after it.  See comment 9 in this bug.
Depends on: 738678
(In reply to Boris Zbarsky (:bz) from comment #41)
> > Do we have STR for the performance issues prior to landing bug 598482
> 
> Just load the testcase in this bug and type.  It takes longer to type the
> same amount of text before that patch than after it.  See comment 9 in this
> bug.

If the difference in performance between the backout in FF12 and the current behavior in FF13 is minimal in any normal user scenario, I agree that there is no reason to continue to back out bug 598482. Let's untrack this for FF13.
tracking-firefox13: + → -
Component: Layout: View Rendering → Layout: Web Painting

Alice,
Is this still an issue?

Flags: needinfo?(alice0775)

No longer reproduced in Noghtly97.0a1 and Firefox91esr.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(alice0775)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: