604455 - Checkin for bug 601440 caused 2.3% Ts regression on 10.4

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Bug 601440 Comment 22 Smokey Ardisson (busy; no bugmail - do not email) 

This caused a 2.3% Ts regression on Camino's 10.4 tinderbox (but not on any of
our 10.5 tinderboxen).  Is this patch doing some sort of work at startup only
on 10.4?

---

601440 Comment 23 Masayuki Nakano (Mozilla Japan)

(In reply to comment #22)
> Is this patch doing some sort of work at startup only
> on 10.4?

No, the patch delays an API call for controlling keyboard layout from "during
focus moving" to "after focus moved".

---

Let me rephrase that question: could any of that code be or cause something that is initialized/set up/whatever at startup (either only on 10.4, or be much slower on 10.4)?  For instance, do we now have to tell the timer service at startup that we're going to need a timer once IME is active, and it needs to start watching, or are we somehow activating IME early on 10.4, or ?

I backed out the patch for bug 601440 locally on the tinderbox, and the Ts values went right back to what they were pre-bug 601440, so the patch is clearly at fault.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Flagging blocking1.9.2? to get this on the radar at least for investigation, since 1.9.2 is the last place we support 10.4, and it seems wrong to saddle those users with a Ts regression in a security release.

Comment 2

[LegNeato]

Yup, I saw it. I wouldn't hold .11 for this but will mark it as blocking for .12 while we investigate.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

The only thing I can guess, Masayuki, is that this regression is
caused by your patch using an nsITimer.  Why not try using the
following OS-specific method, instead?

-[NSObject performSelector:withObject:afterDelay:0]

The object you call this method on will probably need to be
AppShellDelegate (which is sufficiently long-lived -- it lives as long
as the app shell itself).  Which means you'll need to add a method to
AppShellDelegate.

You'll also need to call +[NSObject
cancelPreviousPerformRequestsWithTarget:] in nsAppShell's destructor,
before the call to [mDelegate release].

Comment 4

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

If nsITimer is the cause, I wonder, why this is only on 10.4.

But I'll try the way which is suggested in comment 3.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #3)

> The object you call this method on will probably need to be
> AppShellDelegate

Actually this is probably overkill.  I notice that Cocoa widgets
already uses calls to performSelector:withObject:afterDelay:0 on
ChildView and BaseWindow objects.  Either of these is probably good
enough.

I also notice, though, that neither of these objects' destructors
calls +[NSObject cancelPreviousPerformRequestsWithTarget:self].  They
should :-)

(In reply to comment #4)

> If nsITimer is the cause, I wonder, why this is only on 10.4.

So do I :-)

It was just the best guess I could come with.  But the regression
might also exist on other platforms, and only be visible on 10.4
(presumably because the machines that run 10.4 are older and slower).

Comment 6

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

Attachment: Patch v1.0

Smokey:

Would you test this patch whether this can fix this bug?

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 484279 [details] [diff] [review]
Patch v1.0

I'll try to test this on the tinderbox tomorrow.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 484279 [details] [diff] [review]
Patch v1.0

Unfortunately, this produced no noticeable effect; Ts stayed right where it was after the patch for bug 601440 landed. :(

Comment 9

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

Thank you for your testing.

But seems strange... both SetRomanKeyboardsOnly() and SyncKeyScript() are not called during startup until window gets focus. Are some optimization by compiler gone by the patch??

Comment 10

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

Attachment: Patch v2.0

I guess that KeyScript() on 10.4 is very expensive. After 10.5, backend of keyboard layout was redesigned. So, the legacy code could be slower.

This patch reoptimize SetRomanKeyboardsOnly(). Smokey, could you retest this patch?

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 484970 [details] [diff] [review]
Patch v2.0

(I did wonder if it might have been something with the keyboard script checking, too.)

It'll be a couple of days before I will be able to test this, but I will do so.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 484970 [details] [diff] [review]
Patch v2.0

Yes, this one works :)  The Ts regression on 10.4 was entirely eliminated when I tested this patch.  Thanks, Masayuki!

Comment 13

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

Comment on attachment 484970 [details] [diff] [review]
Patch v2.0

Thank you, Smokey!

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 484970 [details] [diff] [review]
Patch v2.0

> +  static PRBool GetIMEEnabled() { return sIMEEnabled; }

This should return PRUint32.

> +  static PRUint32 sIMEEnabled; // nsIWidget::IME_STATUS_*

It'd be better to rename this variable sIMEEnabledStatus or
sIMEStatus, to avoid confusion with sIsIMEEnabled.

> +void
> +nsTSMManager::SetIMEEnabled(PRUint32 aEnabled)
> +{

This should return nsresult, since it can actually return an error.

And this (from nsChildView::SetIMEEnabled()):

> +  nsTSMManager::SetIMEEnabled(aState);
>    return NS_OK;

Should be:

  return nsTSMManager::SetIMEEnabled(aState);

With these changes, this patch is fine with me.

Comment 15

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

Attachment: Patch v2.1

Sorry for the delay. I'll land this ASAP.

Comment 16

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4f3637c399f1

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Verifying this fix based on the fact our 10.4 Ts numbers have returned to what they were before the checkin for bug 601440 :)

http://build-graphs.mozilla.org/graph/query.cgi?tbox=cb-xserve01.mozilla.com_mozilla_1_9_2_branch&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=&avg=1&days=30

Thanks again!