622536 - OnProgress/OnStatus events processing is very expensive in Remote fennec

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Opreport -c SVG, decodeondraw pref enabled = true

After profiling bugs 622470 and 622475, I found that OnProgress/OnStatus messages processing is very expensive in latest fennec.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: txt version of profile data

Comment 2

[Oleg Romashin (:romaxa)]

I've tested this with methodjit enabled and huge CPU consumption has gone from profile... 

I think this is also slowdown performance for other pages...
Can we enable mjit here? now for fennec? we are using in maemo6 mjit ~1-2 month, and it seems work fine.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: mjit enabled svg graph

Comment 4

[Doug Turner (:dougt)]

Attachment: flip the switch

can't remove because firefox still has this pref off.  Not sure what this is going to break, but I have been runnning with it and haven't noticed anything significant.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 502046 [details] [diff] [review]
flip the switch

I'm fine with trying this again. Let's make sure we don't regress Ts.

Comment 6

[Doug Turner (:dougt)]

http://hg.mozilla.org/mobile-browser/rev/fd2537bf5603

Comment 7

[Doug Turner (:dougt)]

regressed tPan and tShutdown (and mfinkle said it also hurt tS) Backed out changeset fd2537bf5603

Comment 8

[Doug Turner (:dougt)]

mfinkle and I started debuggin more and it looks like we can greatly reduce the number of onprogress and onstatus messages that we need to process.  For example, we can do something like:

diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js
--- a/chrome/content/bindings/browser.js
+++ b/chrome/content/bindings/browser.js
@@ -4,18 +4,22 @@ let Ci = Components.interfaces;
 dump("!! remote browser loaded\n")
 
 let WebProgressListener = {
   _notifyFlags: [],
   _calculatedNotifyFlags: 0,
   _lastLocation: null,
 
   init: function() {
+    let flags = Ci.nsIWebProgress.NOTIFY_LOCATION |
+                Ci.nsIWebProgress.NOTIFY_SECURITY |
+                Ci.nsIWebProgress.NOTIFY_STATE_ALL;
+  
     let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress);
-    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_ALL);
+    webProgress.addProgressListener(this, flags);
 
     addMessageListener("WebProgress:AddProgressListener", this);
     addMessageListener("WebProgress:RemoveProgressListener", this);
   },
 
   onStateChange: function onStateChange(aWebProgress, aRequest, aStateFlags, aStatus) {
     let webProgress = Ci.nsIWebProgressListener;
     let notifyFlags = 0;


In my simple testing, it we go from around a few hundred onProgress|onStatus message for a complex page to, my favorite number, zero.

Comment 9

[Doug Turner (:dougt)]

mfinkle also mentioned that this approach needs to be a bit more robust.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> mfinkle also mentioned that this approach needs to be a bit more robust.

I think I am going to push for a variation of Ben's patch in bug 588441.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 588441 landed and should have reduced the onProgress and onStatus message to zero, as shown by Doug's tests

Comment 12

[Oleg Romashin (:romaxa)]

Great problem is not visible anymore!
I'm able to load http://www.fusker.lv/index.php?lid=286341&offset=30&special=preview
with pref("image.mem.discardable", false); on fennec Maemo6