Closed
Bug 1502284
Opened 6 years ago
Closed 6 years ago
Cleanup of the observer service code
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
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.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
This is introduce for dev tools. Asking Alexander who made bug 1269765.
Flags: needinfo?(dd.mozilla) → needinfo?(poirot.alex)
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
If that's the case I'll gladly drop it entirely, thanks!
Assignee | ||
Comment 5•6 years ago
|
||
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 | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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
Updated•6 years ago
|
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
Updated•6 years ago
|
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
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
No longer blocks: memshrink-content
Whiteboard: [overhead:4k]
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
Comment 10•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee | ||
Comment 11•6 years ago
|
||
I just realized that Lando only landed the patch related to nsMaybeWeakPtr, not the cleanup itself. I'll land it manually.
Comment 12•6 years ago
|
||
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77a41fad0e6
Cleanup of the observer service code r=erahm
Comment 13•6 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•