Closed Bug 1376507 Opened 7 years ago Closed 7 years ago

Use a single watchdog thread for all XPCJSContexts

Categories

(Core :: XPConnect, enhancement)

Product:
Core
Component:
XPConnect
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: billm, Assigned: mrbkap)

References

Details

Attachments

(2 files)

Bug 1376507 - Move state onto XPCJSContext.
(deleted), text/x-review-board-request
billm
: review+
Details
Bug 1376507 - Handle a list of contexts instead of a single context.
(deleted), text/x-review-board-request
billm
: review+
Details
Once we have more than one XPCJSContext, it will be silly to have a separate watchdog thread for each one. I think we might do this by having a single Watchdog instance, and one WatchdogManager per context. Somehow we would need to make WatchdogMain figure out which active context has been active for the longest time to see how long we should sleep for. Blake, can you take this on?
Flags: needinfo?(mrbkap)
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Comment on attachment 8902035 [details] Bug 1376507 - Move state onto XPCJSContext. https://reviewboard.mozilla.org/r/173434/#review179698 This looks good, but the existing locking is not very nice. I'd appreciate it if you could clean it up a bit. ::: js/xpconnect/src/XPCJSContext.cpp:314 (Diff revision 1) > // The watchdog thread always holds the lock when it runs. > Maybe<AutoLockWatchdog> maybeLock; > if (NS_IsMainThread() && mWatchdog) > maybeLock.emplace(mWatchdog); RecordTimestamp is only called from WatchdogMain. Can we just assert that we always hold the lock, and that we're never on the main thread? Also, could we change AutoLockWatchdog to correctly handle a null watchdog? That would be cleaner than using Maybe<> everywhere. ::: js/xpconnect/src/XPCJSContext.cpp:324 (Diff revision 1) > + Maybe<AutoLockWatchdog> maybeLock; > + if (NS_IsMainThread() && mWatchdog) > + maybeLock.emplace(mWatchdog); The way we typically would do this is to pass a |const AutoLockWatchdog& aProofOfLock| as an argument. Anyone calling GetContextTimestamp would have to use AutoLockWatchdog. ::: js/xpconnect/src/XPCJSContext.cpp:335 (Diff revision 1) > Maybe<AutoLockWatchdog> maybeLock; > if (NS_IsMainThread() && mWatchdog) > maybeLock.emplace(mWatchdog); This could be changed to lock similar to GetContextTimestamp.
Attachment #8902035 - Flags: review?(wmccloskey) → review+
Comment on attachment 8902036 [details] Bug 1376507 - Handle a list of contexts instead of a single context. https://reviewboard.mozilla.org/r/173436/#review179734 ::: js/xpconnect/src/XPCJSContext.cpp:424 (Diff revision 1) > + for (auto* context = mActiveContexts.getFirst(); context; > + context = context->LinkedListElement<XPCJSContext>::getNext()) { Can you use a range-based for loop?
Attachment #8902036 - Flags: review?(wmccloskey) → review+
Comment on attachment 8902036 [details] Bug 1376507 - Handle a list of contexts instead of a single context. https://reviewboard.mozilla.org/r/173436/#review179734 > Can you use a range-based for loop? Because `XPCJSContext` now inhertis directly from `LinkedListElement<...>` and `CycleCollectedJSContext` which, itself, inherits from `LinkedListElement<...>` all of the methods inherited from `LinkedListElement` are ambiguous if called directly.
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/04ecce0d1392 Move state onto XPCJSContext. r=billm https://hg.mozilla.org/integration/autoland/rev/266611b269cc Handle a list of contexts instead of a single context. r=billm
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ded0b612dba7 Move state onto XPCJSContext. r=billm https://hg.mozilla.org/integration/autoland/rev/7e5fec0ca1ca Handle a list of contexts instead of a single context. r=billm
Flags: needinfo?(mrbkap)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: