1375491 - IME handling flushes layout too eagerly

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

TryToFlushPendingNotificationsToIME() and ContentEventHandler::InitBasic() both end up flushing layout. Hopefully we could move flushing to happen around refresh driver tick time.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, look fine to me. I test the trybuild for a couple of hours, please wait.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I test it on Win10, Ubuntu17.04 and macOS with various Japanese IMEs. Then, I don't see any problem (except existing bugs, sigh).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ime_flush_at_tick_3.diff

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ime_flush_at_tick_4.diff

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ime_flush_at_tick_5.diff

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

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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!

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ime_flush_at_tick_6.diff

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ime_flush_at_tick_7.diff

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eea0e7fbff9
make child process to cache ime properties only at animation tick time, r=masayuki

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7eea0e7fbff9

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm still not sure, but I find the cause. Actually, the orange detects hidden bug of IME state management in IMEStateManager.