962233 - Rewrite Keyboard chrome jsm code in C++

Status: RESOLVED DUPLICATE of bug 966172

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Sherk (:drs) (inactive)]

Based on my profiling, the supporting chrome code for the Gaia keyboard is taking about 30% of the cycles when opening the keyboard and typing in it. From what I've heard, chrome code is not JIT'd, and even if it was, this kind of code doesn't lend itself very well to JIT optimizations.

This code was a lot easier to write initially in JS because of all the different interfaces that it uses. We have good evidence that rewriting this in C++ will improve performance.

Comment 1

[Dietrich Ayala (:dietrich)]

Can we push any of that logic out into the Web app parts?

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Not really I think. The chrome code only does event dispatching.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

s/dispatching/wiring

Comment 4

[Dietrich Ayala (:dietrich)]

Is there other chrome JS code doing similar event wiring for FxOS? Can we write a general purpose event hub for this instead of keyboard-specific code?

Comment 5

[Doug Sherk (:drs) (inactive)]

(In reply to Dietrich Ayala (:dietrich) from comment #4)
> Is there other chrome JS code doing similar event wiring for FxOS? Can we
> write a general purpose event hub for this instead of keyboard-specific code?

I think it's already about as general as it can get. In fact, probably too much so. There are other problems with it, like the fact that we're broadcasting keyboard messages instead of only sending them between the content and keyboard processes.

Comment 6

[[:fabrice] Fabrice Desré]

Broadcasting is bad indeed. I discussed with mrbkap a better way to do ipc that would remove the need of manual management for "listeners" in these cases, while providing a nice api on the js side and keeping the option to implement in c++ in the parent (because xpcom). I'll file that and cc you all.

Comment 7

[Doug Sherk (:drs) (inactive)]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Broadcasting is bad indeed. I discussed with mrbkap a better way to do ipc
> that would remove the need of manual management for "listeners" in these
> cases, while providing a nice api on the js side and keeping the option to
> implement in c++ in the parent (because xpcom). I'll file that and cc you
> all.

Do you think we should actually block this on bug 962804? I think we could get significant gains still by doing the port to C++ without a better API.

Comment 8

[[:fabrice] Fabrice Desré]

You can go ahead without 962804 - I just thought this would be a reasonably easy testbed. Do you have a cleopatra profile that shows where we spend our overhead?

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> You can go ahead without 962804 - I just thought this would be a reasonably
> easy testbed. Do you have a cleopatra profile that shows where we spend our
> overhead?

Yes, I did a profile of opening the UI test app keyboard section, then typing "test" into one of the input fields.

Here's a profile of the UI test content process:
http://people.mozilla.org/~bgirard/cleopatra/#report=95f542adadcbdaf6693aab5eeb54541cf2e4c237

Here's the keyboard process:
http://people.mozilla.org/~bgirard/cleopatra/#report=947f7e9c11cebdd248d3e8d5eb6b35a597ffc037

Here's the main b2g process:
http://people.mozilla.org/~bgirard/cleopatra/#report=3e406cac9c784ef9b6b63eaf843a34e7b17e82b3

From the profile of the b2g process, it looks like we're spending about 15% of our time in jsm code that looks to me like it could be much, much lower. BrowserElementPanning is also giving us a little performance hit on top of that, but I'm not sure how much that can be improved.

There's a lot going on inside keyboard.js and latin.js on the keyboard process, but this code is very complicated and doesn't lend itself well to being rewritten in C++. I'm not an expert in this code, but I don't see any room for improvement by just rewriting some of that.

Comment 10

[Doug Sherk (:drs) (inactive)]

I'd also like to take this chance to think aloud about something that I think could give us a pretty big perf boost.

The way touch events (non-keyboard ones, such as ones to pan a window) are processed is very similar to keyboard events, but is slightly more refined. In a nutshell, they both go widget->content->x. In the case of touch events, generally x is the compositor, but in the case of keyboard events, x is the keyboard process. I could be wrong in the case of keyboard events because I haven't looked too heavily into this or actually done a full trace from e2e.

Also, to be pedantic, touch events aren't really going to the compositor. They actually go back to the main thread (which is the same as widget on b2g), which mutates some transforms, which the compositor then consumes and is thus the most directly felt change for the user because these touch events will probably pan/zoom the window.

However, we cut corners a bit with touch events by only passing these events to content if content has registered listeners for them. So if content has no listener, the event just goes straight from widget->compositor. The speedup is pretty massive and it feels really sluggish if content has registered listener(s).

If my mental model of how keyboard events get passed around is correct, I don't see any reason that they can't go straight from widget->keyboard, unless content has registered a listener. Hit testing shouldn't be an issue, since widget should know enough about where to forward events based on where they are focused that it doesn't need to round trip anywhere.

Comment 11

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #10)
> I'd also like to take this chance to think aloud about something that I
> think could give us a pretty big perf boost.

Okay, nevermind this whole thing. I looked more at the UI test content process profile and realized that RecvAsyncMessage is just it receiving the key presses, not being round-tripped through. So we're already doing what I said.

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Doug Sherk (:drs) from comment #9)

> From the profile of the b2g process, it looks like we're spending about 15%
> of our time in jsm code that looks to me like it could be much, much lower.
> BrowserElementPanning is also giving us a little performance hit on top of
> that, but I'm not sure how much that can be improved.

It seems to me that out of the 12% spent in keyboardHandleFocusChange() in Keyboard.jsm, about 10 is actually spent in the system app handling the mozChromeEvent. We spend a bit in ObjectWrapper that we could avoid by wrapping manually though.

> There's a lot going on inside keyboard.js and latin.js on the keyboard
> process, but this code is very complicated and doesn't lend itself well to
> being rewritten in C++. I'm not an expert in this code, but I don't see any
> room for improvement by just rewriting some of that.

anyway this is not something where we could ship c++ unless you do c++ -> emscripten.

ButI just spent 30s looking at the b2g profile so feel free to go ahead!

Comment 13

[Dietrich Ayala (:dietrich)]

We need a performant way to write keyboards in JS - would prefer to identify and optimize the parts of the platform that make web app keyboards slow before rewriting in C++ and using Emscripten.

Comment 14

[Doug Sherk (:drs) (inactive)]

(In reply to Dietrich Ayala (:dietrich) from comment #13)
> We need a performant way to write keyboards in JS - would prefer to identify
> and optimize the parts of the platform that make web app keyboards slow
> before rewriting in C++ and using Emscripten.

Well, we have no real evidence that the keyboard logic JS living on the keyboard process is actually significantly slower than if it was written in C++, other than our knowing that JS is generally slower. I think this code is JIT'd and does lend itself fairly well to this.

But we do know that just the handing off of an event should not take ~15% of the time from the widget receiving a touch to actually passing it along to the keyboard, so this seems like a more obvious starting point for improvement.

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #10)
> I'd also like to take this chance to think aloud about something that I
> think could give us a pretty big perf boost.
> 
> The way touch events (non-keyboard ones, such as ones to pan a window) are
> processed is very similar to keyboard events, but is slightly more refined.
> In a nutshell, they both go widget->content->x. In the case of touch events,
> generally x is the compositor, but in the case of keyboard events, x is the
> keyboard process. I could be wrong in the case of keyboard events because I
> haven't looked too heavily into this or actually done a full trace from e2e.
> 
> Also, to be pedantic, touch events aren't really going to the compositor.
> They actually go back to the main thread (which is the same as widget on
> b2g), which mutates some transforms, which the compositor then consumes and
> is thus the most directly felt change for the user because these touch
> events will probably pan/zoom the window.
> 
> However, we cut corners a bit with touch events by only passing these events
> to content if content has registered listeners for them. So if content has
> no listener, the event just goes straight from widget->compositor. The
> speedup is pretty massive and it feels really sluggish if content has
> registered listener(s).
> 
> If my mental model of how keyboard events get passed around is correct, I
> don't see any reason that they can't go straight from widget->keyboard,
> unless content has registered a listener. Hit testing shouldn't be an issue,
> since widget should know enough about where to forward events based on where
> they are focused that it doesn't need to round trip anywhere.

I think I was actually more or less correct here, but not exactly the way that I wrote. Fabrice and I spoke a bit more about this on IRC.

What's actually happening is that key presses are going from widget->content->keyboard. Content is given the chance to intercept and preventDefault the key press on their way to the keyboard. This seems counter-intuitive because the keyboard process, in this case, is responsible for autocompletion and visual changes as a result of touch (for example, highlighting the key you touch). It's not responsible for the gathering of initial spawning of the touch event, since that comes from widget/gonk. If content preventDefaults the touch, we shouldn't show side-effects of the touch on the keyboard, such as altering autocomplete results.

The important detail here is that, if there are no input/key event listeners in content, then the event can't possibly be stopped on its way to the keyboard process. This means that we can fire-and-forget the event to both the keyboard and content processes at the same time instead of waiting for the trip through content before going to the keyboard.

I don't know how much of a perf gain (at least perceptibly) this will give us if done, but I assume it would be at least a bit noticeable. I also don't have stats on how many apps/webpages have event key/input listeners, either. Since some of the machinery the widget->content->keyboard trip is designed for handling event listeners, we can circumvent those in the no-listeners case and maybe get a perf boost there, as well.

For anyone interested, I recommend looking at this pseudo-trace through the IPC of a key press: https://bugzilla.mozilla.org/show_bug.cgi?id=921299#c7

If we are to do this, I'm not actually sure how to structure it so that it's sane. We need to know across processes whether or not there are input/key listeners on any content in that process. We can gather that information at https://mxr.mozilla.org/mozilla-central/source/dom/events/nsIEventListenerService.idl, but there's no machinery to transmit this across processes (yet).

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Is this bug still valid? What's the action item here?

Comment 17

[Doug Sherk (:drs) (inactive)]

I think this is basically a dud. But the work in bug 966172, which is an offshoot of the discussions here, is still useful.