Closed Bug 920804 Opened 6 years ago Closed 6 years ago

Improve nsFrameMessageManager

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
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)

Attached patch wip patch (obsolete) — 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)
We have the patch for this somewhere. reviewed patch even, I think.
Or maybe not. At least I can't find it. But jlebar certainly wrote something like this.
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+
Cleaned up and complete patch sent to try:

https://tbpl.mozilla.org/?tree=Try&rev=77543ea0498d
Attached patch patch (obsolete) — Splinter Review
With less ws, but as much test failures:

https://tbpl.mozilla.org/?tree=Try&rev=d8b772a60e60
Attachment #810201 - Attachment is obsolete: true
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.
Attached patch message-manager.patch (obsolete) — Splinter Review
Olli, If you could help with the try failure that would be great!
Attachment #810589 - Attachment is obsolete: true
Attachment #814453 - Flags: feedback?(bugs)
Status: NEW → ASSIGNED
Keywords: perf
Whiteboard: [c= p= s= u=]
Attachment #814453 - Flags: feedback?(bugs) → review?(bugs)
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).
I assume that will still fail. I don't understand...
Attached patch mm_hash_2.diffSplinter Review
Attachment #814453 - Attachment is obsolete: true
Attachment #814453 - Flags: review?(bugs)
Attachment #821399 - Flags: review?(fabrice)
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)
Attachment #821399 - Flags: review?(fabrice) → review+
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
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.
Attached patch mm_hash_3.diffSplinter Review
Note, -w of v2 and v3 is exactly the same.
https://hg.mozilla.org/mozilla-central/rev/cbbbadec7f55
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
We need to backport this patch to 1.2 to fix bug 937394.
Blocks: 937394
blocking-b2g: --- → koi?
koi+ blocking a blocker
blocking-b2g: koi? → koi+
This doesn't apply cleanly to b2g26. Please post a branch-specific patch or request uplift for whatever else this depends on.
Flags: needinfo?(fabrice)
Ryan, can you check if https://bugzilla.mozilla.org/attachment.cgi?id=8341582 from bug 933711 applies cleanly?
Flags: needinfo?(fabrice)
It does not.
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.