Add a preference variable cache for the pref checks within WinIMEHandler

RESOLVED WONTFIX

Status

()

Core
Widget: Win32
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: jaws, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Blocks: 1192573
No longer depends on: 1192573
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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Created attachment 8651952 [details] [diff] [review]
Patch
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8651952 - Flags: review?(masayuki)
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?
Attachment #8651952 - Flags: review?(masayuki) → review-
(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.
Fair enough.
Assignee: jaws → nobody
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.