765135 - TISCreateInputSourceList causes Main Thread IO (200ms after startup)

Status: NEW

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

Description

[Benoit Girard (:BenWa)]

TISCreateInputSourceList is causing some main thread IO:
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/TextInputHandler.mm#402

https://developer.apple.com/library/mac/#documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html

Profile:
http://people.mozilla.com/~bgirard/cleopatra/?report=AMIfv979Ar1NgD17oweVdFUTVb50HKEHhDmV2_4e6yTWklFjU63YQ8cJyeCm2O_4aJ7dqrOY1QGF3ewnSe2O5-eNLlripwpiIadeKciM8_Y45NXJ_TNB4oP6BMbVIU5r3Jg_-M0FPgysRExnDCbGX4Fck580knEvUA

Comment 1

[Benoit Girard (:BenWa)]

Corrected to about 200ms by zooming out in the profile selection (not linked above)

Comment 2

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

Attachment: Patch

How about this? This patch caches US keyboard layout for handling shortcut keys. The shortcut key handler is the only caller calling InitByInputSourceID() in normal usage. The other caller is API for automated tests.

Comment 3

[Benoit Girard (:BenWa)]

Any idea what to expect to call this for automated testing? We may automatically look for IO during testing.

Will this patch also remove IO for other non English local?

Comment 4

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

(In reply to Benoit Girard (:BenWa) from comment #3)
> Any idea what to expect to call this for automated testing? We may
> automatically look for IO during testing.

It's used for testing the native key event handling code in any layouts.

I don't think that we should optimize the testing performance with complicated code or wasting memory never used actually because the performance isn't so bad and the tests are not so many.

> Will this patch also remove IO for other non English local?

Yes. US keyboard layout is used for detecting some keyboard layouts which switch the keys when Cmd key is pressed like Dvorak-QWERTY. So, it's being used with any keyboard layouts.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Benoit, does Masayuki's patch actually stop TISCreateInputSourceList() being called on startup?  (TISCreateInputSourceList() must still be called once, and I don't know if Masayuki's patch changes when that call happens.)

And is it possible to extract a stack trace of the call(s) to TISCreateInputSourceList() from your profile (in comment #0)?

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #5)
> Benoit, does Masayuki's patch actually stop TISCreateInputSourceList() being
> called on startup?  (TISCreateInputSourceList() must still be called once,
> and I don't know if Masayuki's patch changes when that call happens.)
> 
> And is it possible to extract a stack trace of the call(s) to
> TISCreateInputSourceList() from your profile (in comment #0)?

I've been wanting this as well. Bug 765813

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Actually it took me a while even to *find* the call to TISCreateInputSourceList().  I ended up setting the "filter" to "TISCreateInputSourceList", then laboriously expanding each individual level in the stack trace.  (So Cleopatra should also support "expand all".)

What I found puzzles me:  TISCreateInputSourceList() is called (indirectly) from [nsChildView keyDown:] -- in other words on processing a native keyDown event.  This isn't something you'd normally expect to happen on startup.

Do you have any idea, Benoit, why it happened while you were profiling?

Could you try profiling again, and this time make sure you don't hit any keyboard keys while it's happening?

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> [nsChildView keyDown:]

[ChildView keyDown:]

Comment 9

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

Benoit: Could you check comment 7?

Comment 10

[Benoit Girard (:BenWa)]

Alright so the problem still occurs but I only see the execution happening when I press a key which I often do to save a profile.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> I only see the execution happening when I press a key which I often
> do to save a profile

You're doing this on startup?

I still don't understand.

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #11)
> > I only see the execution happening when I press a key which I often
> > do to save a profile
> 
> You're doing this on startup?
> 
> I still don't understand.

This shows up when you do your first keypress. My startup profiling workflow caught it because I would restart the browser and press APPLE+SHIFT+O to open the profile.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 634005 [details] [diff] [review]
Patch

Does this still make sense?  Is this still wanted?

Comment 14

[Benoit Girard (:BenWa)]

We still want a solution to this problem. I don't know enough to review this patch.

Comment 15

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

I guess that the patch still works. I'll confirm it ASAP.

Comment 16

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

Moving all open Keyboard/IME handling bugs to DOM: UI Events & Focus Handling component.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Sorry for taking so very long to get back to this. Masayuki, is your patch still relevant?

Comment 18

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

I think that some cost is anyway required by first key press. Looks like my patch reduces the cost at 2nd and later key presss.

In these days, we have better runnable APIs, so, we could do the first call while idle time immediately after starting up. However, it anyway would block the main thread and user might try to do something at the moment. So I don't think this approach is reasonable unless the cost is too expensive which causes users feel Firefox slow.

During the long delay in this bug, the storage has already been changed from SATA HDD/SSD to NVMe SSD and macOS (MacOS X) upgraded a lot, so I'm not sure whether this bug is still valid or not.