IME handling flushes layout too eagerly

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

TryToFlushPendingNotificationsToIME() and ContentEventHandler::InitBasic() both end up flushing layout. Hopefully we could move flushing to happen around refresh driver tick time.
Posted patch wip (obsolete) — Splinter Review
Very much untested
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/853d4964b88f39d9cd06693af6153eb417982651
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=853d4964b88f39d9cd06693af6153eb417982651
Masayuki, the patch is just a wip, sort of "could we do something like this to flush layout less often".
Do you think you or someone else who actively uses IME could take a look?
I did some small testing on Windows, and the candidate window wasn't at totally wrong location :)


(One obvious issue if refreshdriver was used for updates this way is that IMENotificationSender shouldn't keep strong reference to IMEContentObserver, but nsWeakPtr)
Flags: needinfo?(masayuki)
Blocks: 1376334
Thanks, look fine to me. I test the trybuild for a couple of hours, please wait.
Flags: needinfo?(masayuki)
I test it on Win10, Ubuntu17.04 and macOS with various Japanese IMEs. Then, I don't see any problem (except existing bugs, sigh).
Posted patch v2 (obsolete) — Splinter Review
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/b2746aa20bcd804746dd9f81f595e2764c57c031
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2746aa20bcd804746dd9f81f595e2764c57c031
Attachment #8881270 - Attachment is obsolete: true
Posted patch ime_flush_at_tick_3.diff (obsolete) — Splinter Review
Non-IME parts are just renamings, expect that one test change.
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/c1af2c371e2b90129a0e5693bf71a2e783a10ec4
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1af2c371e2b90129a0e5693bf71a2e783a10ec4
Assignee: nobody → bugs
Attachment #8881383 - Attachment is obsolete: true
Attachment #8881470 - Flags: review?(masayuki)
Comment on attachment 8881470 [details] [diff] [review]
ime_flush_at_tick_3.diff

Excellent!

> void
> IMEContentObserver::TryToFlushPendingNotifications()
> {
>-  if (!mQueuedSender || mSendingNotification != NOTIFY_IME_OF_NOTHING) {
>+  if (!mQueuedSender || mSendingNotification != NOTIFY_IME_OF_NOTHING ||
>+      XRE_IsContentProcess()) {
>     return;
>   }

This is really tricky. I think that both IMEContentObserver.h and EventStateManager.h should explain this fact.

> NS_IMETHODIMP
> IMEContentObserver::IMENotificationSender::Run()
> {
> ...
>+      observer->mQueuedSender =
>+        new IMENotificationSender(observer);
>+      observer->mQueuedSender->
>+        Dispatch(observer->mDocShell);

can be:

>      observer->mQueuedSender = new IMENotificationSender(observer);
>      observer->mQueuedSender->Dispatch(observer->mDocShell);

>+      observer->mQueuedSender =
>+        new IMENotificationSender(observer);
>+      observer->mQueuedSender->
>+        Dispatch(observer->mDocShell);

Same.

>     }
>   }
>   return NS_OK;
> }


I have some questions. I'm not really familiar with refresh driver. I'd like to know:

1. How often does it try to refresh the content?
2. Does it try to refresh before next event loop? I mean, if an input event causes DOM tree change, then, does it try to refresh before next event loop?
3. Are there something we shouldn't do which cause refresh driver working more frequently and making damage to the performance?
Attachment #8881470 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> > void
> > IMEContentObserver::TryToFlushPendingNotifications()
> > {
> >-  if (!mQueuedSender || mSendingNotification != NOTIFY_IME_OF_NOTHING) {
> >+  if (!mQueuedSender || mSendingNotification != NOTIFY_IME_OF_NOTHING ||
> >+      XRE_IsContentProcess()) {
> >     return;
> >   }
> 
> This is really tricky. I think that both IMEContentObserver.h and
> EventStateManager.h should explain this fact.
I'll add some comment


> I have some questions. I'm not really familiar with refresh driver. I'd like
> to know:
> 
> 1. How often does it try to refresh the content?
If there has been changes requiring style or layout flush, or some animations or such, every 16ms

> 2. Does it try to refresh before next event loop? I mean, if an input event
> causes DOM tree change, then, does it try to refresh before next event loop?
No. Refresh driver ticks based on vsync messages from gpu process/thread

> 3. Are there something we shouldn't do which cause refresh driver working
> more frequently and making damage to the performance?
refresh driver never ticks faster than event loop spins.
Posted patch ime_flush_at_tick_4.diff (obsolete) — Splinter Review
Posted patch ime_flush_at_tick_5.diff (obsolete) — Splinter Review
a hack for UIEvent::RangeOffset() issue which this patch may have revealed.
The issue there is old.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/df10c7abd15409d74ad1adbcfc7848b5525f87fd
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=df10c7abd15409d74ad1adbcfc7848b5525f87fd
remote: recorded changegroup in replication log in 0.123s
Attachment #8881470 - Attachment is obsolete: true
Attachment #8881743 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #8)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> > I have some questions. I'm not really familiar with refresh driver. I'd like
> > to know:
> > 
> > 1. How often does it try to refresh the content?
> If there has been changes requiring style or layout flush, or some
> animations or such, every 16ms
> 
> > 2. Does it try to refresh before next event loop? I mean, if an input event
> > causes DOM tree change, then, does it try to refresh before next event loop?
> No. Refresh driver ticks based on vsync messages from gpu process/thread
> 
> > 3. Are there something we shouldn't do which cause refresh driver working
> > more frequently and making damage to the performance?
> refresh driver never ticks faster than event loop spins.

Thank you, really helpful information for me!
Trying this.
The only change is the flush in UIEvent::RangeOffset.
I'm not too happy with that.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/9f36a2c1cda637963ae7347a3eba8cb324770ce8
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f36a2c1cda637963ae7347a3eba8cb324770ce8
remote: recorded changegroup in replication log in 0.099s
Attachment #8881745 - Attachment is obsolete: true
Attachment #8881830 - Flags: review?(masayuki)
Comment on attachment 8881830 [details] [diff] [review]
ime_flush_at_tick_6.diff

I really don't know what RangeOffset() and RangeParent() mean.

However, only RangeOffset() uses nsIFrame and converting point to content offset in it. So, flushing it is necessary. I don't understand that why this works fine without flushing layout in current build, though.

(mPresContext is set to nullptr only in DuplicatePrivateData(). I assume that it won't be called during the flush.)
Attachment #8881830 - Flags: review?(masayuki) → review+
Still no luck. Adding flush also to GetRangeParent as it is needed.

remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=97efffb8ee8538897eaede4d40ba2131a9de701e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=97efffb8ee8538897eaede4d40ba2131a9de701e
remote: recorded changegroup in replication log in 0.128s
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eea0e7fbff9
make child process to cache ime properties only at animation tick time, r=masayuki
https://hg.mozilla.org/mozilla-central/rev/7eea0e7fbff9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
What would happen if we did the same also in parent process? When I'm profiling typing to locationbar, the flush caused by IMEContentObserver takes some time.
Flags: needinfo?(masayuki)
I'm still not sure, but I find the cause. Actually, the orange detects hidden bug of IME state management in IMEStateManager.
Flags: needinfo?(masayuki)
Depends on: 1385666
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.