TimeoutManager::SetTimeout should cache all preferences it needs

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: bkelly)

Tracking

({perf})

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
I'm looking at perf log where TimeoutManager::SetTimeout leads to a lot of
hashtable lookups taking time, via a mozilla::Preferences::GetInt call.
I think it's this one (because it's the only one I can find in SetTimeout):
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/dom/base/TimeoutManager.cpp#455

We should use Preferences::Add...VarCache there.
(Assignee)

Comment 1

a year ago
I'll look tomorrow.  We already use cached prefs in this file, so it should be an easy fix.
Flags: needinfo?(bkelly)
(Assignee)

Comment 2

a year ago
Created attachment 8880404 [details] [diff] [review]
Make TimeoutManager use a pref cache variable for dom.disable_open_click_delay. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=781fbdfc086a7f80b5c147ca1ac96280fff56c99
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
(Assignee)

Comment 3

a year ago
Comment on attachment 8880404 [details] [diff] [review]
Make TimeoutManager use a pref cache variable for dom.disable_open_click_delay. r=ehsan

Avoid pref lookups in setTimeout() by using a cached var.  Pretty straightforward.
Attachment #8880404 - Flags: review?(ehsan)

Updated

a year ago
Whiteboard: [qf] → [qf:p3]

Comment 4

a year ago
Comment on attachment 8880404 [details] [diff] [review]
Make TimeoutManager use a pref cache variable for dom.disable_open_click_delay. r=ehsan

Review of attachment 8880404 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/TimeoutManager.cpp
@@ +342,5 @@
>                                 DEFAULT_MAX_CONSECUTIVE_CALLBACKS_MILLISECONDS);
> +
> +  Preferences::AddIntVarCache(&gDisableOpenClickDelay,
> +                              "dom.disable_open_click_delay",
> +                              0);

Since you defined DEFAULT_DISABLE_OPEN_CLICK_DELAY I expect you meant to use it here instead of hardcoding 0.
Attachment #8880404 - Flags: review?(ehsan) → review+

Comment 5

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39e7022a80c
Make TimeoutManager use a pref cache variable for dom.disable_open_click_delay. r=ehsan

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c39e7022a80c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.