1194369 - Add a preference variable cache for the pref checks within WinIMEHandler

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1192573 added a call to Preferences::GetBool and there already existed a few other pref checks within this code. The code runs whenever a field gets focused, so we should add a cache here so we're not hitting the prefs at each focus change.

Comment 1

[Justin Dolske [:Dolske]]

Do we know this is actually expensive? Fine to cache it if that's the simple and clean thing to do anyway, but I suspect we have a lot of other code that checks prefs at least as frequently, without measurable perf overhead.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Unfortunately I think it's one of those things when measured independently will appear to have a negligible effect, but with many (hundreds to thousands) of preference calls in a quick loop will build up. It's pretty simple to use a pref cache so I think we should go forward with this bug.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 4

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

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

>+  if (!sSetupPrefCache) {
>+    sSetupPrefCache = true;

No, you don't need to do this. IMEHandler::Initialize() is called only once when first nsWindow is created.

>+    Preferences::AddBoolVarCache(&sIsOskEnabled, kOskEnabled, true);
>+    Preferences::AddBoolVarCache(&sShouldDetectPhysicalKeyboard, kOskDetectPhysicalKeyboard, true);
>+    Preferences::AddBoolVarCache(&sIsTabletModeRequired, kOskRequireTabletMode, true);

nit:

    Preferences::AddBoolVarCache(&sIsOnScreenKeyboardEnabled,
                                 kOskEnabled, true);
    Preferences::AddBoolVarCache(&sShouldDetectPhysicalKeyboard,
                                 kOskDetectPhysicalKeyboard, true);
    Preferences::AddBoolVarCache(&sIsTabletModeRequired,
                                 kOskRequireTabletMode, true);

>+  static bool sIsOskEnabled;

sIsOnScreenKeyboardEnabled is better.

>+  static bool sShouldDetectPhysicalKeyboard;
>+  static bool sIsTabletModeRequired;

These two variables are not clear that they are used for automatically open/close onscreen keyboard. Could you add simple comments for explaining that?

Comment 5

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Unfortunately I think it's one of those things when measured independently
> will appear to have a negligible effect, but with many (hundreds to
> thousands) of preference calls in a quick loop will build up. It's pretty
> simple to use a pref cache so I think we should go forward with this bug.

Sure, but you're not running hundreds-to-thousands of calls in a loop, are you? Comment 0 says this just happens when focus shifts into a field? (And even then, as I noted in comment 1, my recollection is that in places we _do_ have pref checks in a relative hot path, the overhead is still negligible.)

See also "premature optimization" and Knuth's views on it.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Fair enough.