Closed Bug 1502284 Opened 6 years ago Closed 6 years ago

Cleanup of the observer service code

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

The observer service code could use some cleanups before I proceed to introduce the new interface in bug 1435683. Some of its code can be factored to make it less verbose and there's some odd stuff that might be removed if it's not needed anymore.
I've stumbled upon this code that "filters" certain topics related to Necko within the observer service implementation: https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/xpcom/ds/nsObserverService.cpp#213 Dragana is this something that's still needed? If it is then I'd like to take it out of the base observer service and move it to the Necko code by wrapping the observer service in a Necko-specific object that would do this filtering.
Flags: needinfo?(dd.mozilla)
This is introduce for dev tools. Asking Alexander who made bug 1269765.
Flags: needinfo?(dd.mozilla) → needinfo?(poirot.alex)
The original filtering isn't related to bug 1269765, but to bug 806753. And I think it was to avoid confusion with addons trying to listen for these necko events from the child process. I imagine you could do what you said and move this to necko. You may also avoid emitting the events if that's any easier. I'm not sure it is critical anymore to have an exeption on listening. You might as well be able to just drop this safety code if that was here only to protect against legacy addons now that they are no more!
Flags: needinfo?(poirot.alex)
If that's the case I'll gladly drop it entirely, thanks!
Depends on: 1506264
Blocking bug 1436250 since modifying the way observer lists store listeners can save some memory. In my tests this is up to ~16KiB in the main process and ~4KiB in content processes.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Whiteboard: [overhead:4k]
This adds a way to detect if an instance is holding a weak reference or a strong one, makes non-critical failures less chatty and adds separate methods for adding unique and non-unique instances to an array.
This refactors the observer service code to improve readability, uses MOZ_TRY and other checking macros wherever possible to simplify error handling and replaces the ObserverRef class with the more generic nsMaybeWeakPtr class. This cuts away some code and halves the amount of memory needed to store an event listener. The external behavior is almost unchanged save for some error codes which are now more specific. Depends on D11645
Attachment #9024438 - Attachment description: Bug 1502284 - Extend nsMaybeWeakPtr and make expose it to the rest of the code → Bug 1502284 - Extend nsMaybeWeakPtr and expose it to the rest of the code
Attachment #9024438 - Attachment description: Bug 1502284 - Extend nsMaybeWeakPtr and expose it to the rest of the code → Bug 1502284 - Extend nsMaybeWeakPtr and make expose it to the rest of the code
I've updated the patches with the review comments addressed. For now I've settled with a bool within the nsMaybeWeakPtr structure to reduce the number of calls to do_QueryInterface() (especially during GC/CC; I hadn't realized it but it was quite noticeable over a large number of listeners). I'll split the memory consumption improvements into a separate bug so that we can land this.
No longer blocks: memshrink-content
Whiteboard: [overhead:4k]
Blocks: 1507434
Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/717c99f90176 Extend nsMaybeWeakPtr and make expose it to the rest of the code r=erahm
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
I just realized that Lando only landed the patch related to nsMaybeWeakPtr, not the cleanup itself. I'll land it manually.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: