Closed Bug 174823 Opened 17 years ago Closed 9 years ago

Re-enable async reflow and painting for text widgets


(Core :: Editor, defect, P4)




Tracking Status
blocking2.0 --- betaN+


(Reporter: kinmoz, Assigned: ehsan)


(Blocks 3 open bugs)


(Keywords: perf)


(1 file, 2 obsolete files)

Attachment 87307 [details] [diff] (async reflow and painting for text widgets), which landed as
part of bug 141900, was temporarily disabled with attachment 101640 [details] [diff] [review] during
mozilla1.2beta because of some problems it exposed, which fell into 3 general

  1. Typed text lags in text widgets, especially in the URL bar. (Bug 158782)
  2. Caret flashes at start of line when deleting. (Bug 151882)
  3. Updating textfields via JS causes flashing. (Bug 165130)

Though bugs 158782, 151882, and 165130 are currently marked as fixed, they will
have to be addressed properly before backing out attachment 101640 [details] [diff] [review] to re-enable
async reflows and paints, so they will be listed as blockers for this bug.

Note that item #1 is not much of a problem on Win32 platforms because of the fix
for bug 163528 which forces syncronous reflows and paints at the widget level
for better dhtml performance.
Depends on: 151882, 158782, 165130
Priority: -- → P3
Target Milestone: --- → mozilla1.3beta
Target Milestone: mozilla1.3beta → mozilla1.4beta
Here's an excerpt from an email I sent to that explains why
some of these issues were happening ...

Issues #1 and #3 seem to be related to the speed of the processor and 
the priority in which PLEvent and OS events are processed on each 
platform. Issue #1 is also more visible in auto-complete textfields such 
as the URL bar, which I'm guessing is due in part to the lookup timer it 
uses. has done some work to adjust the PLEvent/OSEvent 
priorities on Win32 to make the app more responsive and prevent paint 
starvation to improve dhtml performance. Some of his fixes, like the one 
for bug 163528, hide the lag on Win32 platforms because there is now 
some code at the widget level that forces syncronous reflows and paints 
while processing certain events. As far as I'm aware, there is no one 
signed up yet to do this same kind of work on Linux or Mac, which is 
where the majority of the complaints are coming from.

It should be noted that though the text doesn't appear immediately as 
you type, it appears as soon as you stop typing or slow down enough so 
that the paint events can be processed between keystrokes, no matter how 
much text is present in the widget. This seems to be the thing that 
disturbs people the most.

With this patch turned off, there will be situations where you can type 
ahead of what is displayed on screen (once again more apparent in the 
URL bar textfield), but the difference will be that when you stop, 
you'll have to wait till the textfield catches up, reflowing and 
painting one character at a time.

Issue #2 is due to the fact that caret show/hide code is synchronous, 
and to complicate things even further, there are several places in 
layout and editor that hide/show the caret during a single edit 
operation up to 3-4 times. I started to work on cleaning some of this up 
as part of bug 151882, which does have a patch, but the issue of 
sync'ing up the caret with async reflows and paints still has to be 
addressed. I've got some ideas on how to fix the async issue, but it's 
nothing I want to try to implement/land for 1.2.

All this said, I've posted a one line patch (attachment 101640 [details] [diff] [review]) in bug 
141900, which simply avoids setting a bit on the editor, which disables 
attachment 87307 [details] [diff] [review] completely.

With this patch text widgets will still be slightly faster than those 
used in the Mozilla 1.0 release, due to some of the other patches that 
landed as part of bug 141900, but slightly slower than those in Mozilla 
1.1 as mentioned above.
Keywords: perf
Blocks: 188318
Blocks: 83841
Some of the issues this bug ran into could be fixed by bug 244366.
Depends on: 244366
So I just tried doing this....  Now that bug 244366 is fixed, I had no issues with bug 165130.  I really don't type fast enough to tell about bug 158782.  And I couln't reproduce bug 151882 as described, but I _did_ see a tad of caret flicker while quickly typing random stuff in a textarea.  The caret flickered to somewhere about in the middle of the next-to-last line.

Blake, roc, any ideas?  I'd really like to make this change if we can, and we're _so_ close.

Note that editor does use NS_VMREFRESH_DEFERRED when reenabling updates.  I could make it do NO_SYNC and that might help, but would lose the point, since it would cause reflows to flush out at every character...
Flags: blocking1.9a2?
Assignee: kinmoz → mozeditor
QA Contact: sujay
Priority: P3 → --
Target Milestone: mozilla1.4beta → ---
Flags: blocking1.9+
we'd really like this.  roc, kbap can you tell us how close this is?
Caret flicker may have improved with mrbkap's big caret fix. I haven't tried this myself so I can't say anything firsthand. Can you recruit someone to test by backing out attachment 101640 [details] [diff] [review] and seeing how it goes?
> Caret flicker may have improved with mrbkap's big caret fix.

It didn't.  I tested back when that landed.
Flags: blocking1.9a2?
QA Contact: editor
Assignee: mozeditor → nobody
Blake, can you please dig into this?
Assignee: nobody → mrbkap
So, I can't seem to reproduce any sort of caret flicker on Linux. I'll try OS X when I update my build.
This worksforme on Linux too, with current trunk.  Let's give it a shot?
Attached patch Let's try it, then (obsolete) — Splinter Review
Attachment #271175 - Flags: superreview?(roc)
Attachment #271175 - Flags: review?(roc)
Attachment #271175 - Flags: superreview?(roc)
Attachment #271175 - Flags: superreview+
Attachment #271175 - Flags: review?(roc)
Attachment #271175 - Flags: review+
Fix checked into trunk.
Closed: 13 years ago
Resolution: --- → FIXED
I had to back this out -- it was causing reftests to fail.
Resolution: FIXED → ---
Attached file Tinderbox log of test failures (obsolete) —
That's mochitest... but in any case.  That looks like focusing the form control didn't put the caret after all the content, and the rest of the failures follow.  I guess caret placement depends on the frame tree, right?  Perhaps we should flush reflow before trying to move the caret...
Yeah, sorry. I was busy trying to deal with other orange as well. I figured that we probably need to flush layout somewhere -- I'll investigate tomorrow.
Blake, when is that 'tomorrow' ;)
Whiteboard: [dbaron-1.9:RzCe]
Doesn't look like we are getting anywhere and this pre-dates 1.9.  Moving to the wanted list - re-nom if you disagree..
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
Blocks: 586662
This should block because it blocks bug 586662.

mrbkap, I trust that you're not still working on this...
Assignee: mrbkap → ehsan
blocking2.0: --- → ?
Attached patch Patch (v1)Splinter Review
I ran the entire test suite locally on Windows with this patch, and it seems pretty safe.  I also couldn't see any manifestation of bug 158782, bug 151882 or bug 165130 with this patch.  In addition, this indeed seems to fix the problem in bug 586662, so I think we really want to take this.
Attachment #271175 - Attachment is obsolete: true
Attachment #271604 - Attachment is obsolete: true
Attachment #477820 - Flags: review?(roc)
Attachment #477820 - Flags: approval2.0?
Whiteboard: [dbaron-1.9:RzCe] → [needs landing]
Closed: 13 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.