OnProgress/OnStatus events processing is very expensive in Remote fennec

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: romaxa, Assigned: mfinkle)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(4 attachments)

Reporter

Description

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

Comment 2

9 years ago
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.
Reporter

Comment 3

9 years ago
Reporter

Updated

9 years ago
Depends on: 608956
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.
Assignee: nobody → doug.turner
Attachment #502046 - Flags: superreview?(mark.finkle)
Attachment #502046 - Flags: review?(gal)
tracking-fennec: --- → ?
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.
Attachment #502046 - Flags: superreview?(mark.finkle) → superreview+
http://hg.mozilla.org/mobile-browser/rev/fd2537bf5603
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
regressed tPan and tShutdown (and mfinkle said it also hurt tS) Backed out changeset fd2537bf5603
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
mfinkle also mentioned that this approach needs to be a bit more robust.
(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.
Depends on: 588441
Attachment #502046 - Flags: superreview-
Attachment #502046 - Flags: superreview+
Attachment #502046 - Flags: review?(gal)
Attachment #502046 - Flags: review-
Assignee: doug.turner → mark.finkle
tracking-fennec: ? → 2.0+
Bug 588441 landed and should have reduced the onProgress and onStatus message to zero, as shown by Doug's tests
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Reporter

Comment 12

9 years ago
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
You need to log in before you can comment on or make changes to this bug.