Closed
Bug 920804
Opened 11 years ago
Closed 11 years ago
Improve nsFrameMessageManager
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
Tracking | Status | |
---|---|---|
firefox27 | --- | fixed |
b2g-v1.2 | --- | fixed |
b2g-v1.3 | --- | unaffected |
People
(Reporter: fabrice, Assigned: fabrice)
References
Details
(Keywords: perf, Whiteboard: [c= p= s= u=])
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Currently the message manager keeps a the message listeners in a simple array, and as such needs to iterate over the full array to find potential listener for a message.
This patch replaces this array by a hash table indexed on the message name, so we get O(1) access to the list of matching listeners. With this patch I see the overhead in nsFrameMessageManager::ReceiveMessage() now very constantly under 1ms, with some outliers cases that I'm still investigating.
Attachment #810201 -
Flags: feedback?(bugs)
Comment 1•11 years ago
|
||
We have the patch for this somewhere. reviewed patch even, I think.
Comment 2•11 years ago
|
||
Or maybe not. At least I can't find it. But jlebar certainly wrote something like this.
Comment 3•11 years ago
|
||
Comment on attachment 810201 [details] [diff] [review]
wip patch
Yeah, I think we need to have something like this.
MessageManager is being used in a bit different way than I originally thought :)
Remove the /**/ and fix the TODO etc.
And -w patch will help reviewing.
Attachment #810201 -
Flags: feedback?(bugs) → feedback+
Assignee | ||
Comment 4•11 years ago
|
||
Cleaned up and complete patch sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=77543ea0498d
Assignee | ||
Comment 5•11 years ago
|
||
Fixing unused variables:
https://tbpl.mozilla.org/?tree=Try&rev=e48b47dddb04
Assignee | ||
Comment 6•11 years ago
|
||
With less ws, but as much test failures:
https://tbpl.mozilla.org/?tree=Try&rev=d8b772a60e60
Attachment #810201 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
I added some logging to understand why M-1 is failing. It seems that Specialpowers sync calls to getBoolPref() is failing because no listener is found for the SPPrefService message even though it's added correctly. I'm not sure yet how this patch breaks the world.
Assignee | ||
Comment 8•11 years ago
|
||
Olli, If you could help with the try failure that would be great!
Attachment #810589 -
Attachment is obsolete: true
Attachment #814453 -
Flags: feedback?(bugs)
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Attachment #814453 -
Flags: feedback?(bugs) → review?(bugs)
Comment 9•11 years ago
|
||
I'm having hard time to see other problems than leaking listeners in unlink
(need to actually delete the arrays, and that leads to deleting the nsMessageListenerInfo objects).
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
I assume that will still fail. I don't understand...
Comment 12•11 years ago
|
||
Attachment #814453 -
Attachment is obsolete: true
Attachment #814453 -
Flags: review?(bugs)
Attachment #821399 -
Flags: review?(fabrice)
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
Fabrice, so the main difference to your patch is to fix ReceiveMessage to not return early
in case there are no listeners, but to call the parent message manager,
and to make memory reporter stuff working with the new setup.
(I also fixed the coding style of the recently landed memory reporter code)
Assignee | ||
Updated•11 years ago
|
Attachment #821399 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
Unfortunately had to back this out as it conflicted against https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c43a39c0f6 when mozilla-central was merged to b2g-inbound.
Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/fb0173b73836
Comment 17•11 years ago
|
||
Fabrice, could you update the patch. I may not be online too much today.
Changes in
https://hg.mozilla.org/integration/mozilla-inbound/diff/e4c43a39c0f6/content/base/src/nsFrameMessageManager.cpp are trivial.
Comment 18•11 years ago
|
||
Note, -w of v2 and v3 is exactly the same.
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 21•11 years ago
|
||
We need to backport this patch to 1.2 to fix bug 937394.
Blocks: 937394
blocking-b2g: --- → koi?
Comment 23•11 years ago
|
||
This doesn't apply cleanly to b2g26. Please post a branch-specific patch or request uplift for whatever else this depends on.
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → unaffected
status-firefox27:
--- → fixed
Flags: needinfo?(fabrice)
Keywords: branch-patch-needed
Assignee | ||
Comment 24•11 years ago
|
||
Ryan, can you check if https://bugzilla.mozilla.org/attachment.cgi?id=8341582 from bug 933711 applies cleanly?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 26•11 years ago
|
||
Flags: needinfo?(fabrice)
Updated•11 years ago
|
Keywords: branch-patch-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•