smooth out status notifications

RESOLVED FIXED

Status

()

Core
General
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

(Depends on: 1 bug, {perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
We send a lot of notifications for status text and progress bars. With the rise of Web 2.0, many sites rely on several so-called widgets to populate a page. This makes our progress go backwards. On techcrunch.com, our status bar changes directions 5 or 6 times.

This patch decreases notification frequency, sends status text only when there is new text, and prevents status bars from moving backwards.
(Assignee)

Comment 1

11 years ago
Created attachment 283812 [details] [diff] [review]
smooth out status notifications
Assignee: nobody → sayrer
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #283812 - Flags: review?(cbiesinger)
(Assignee)

Updated

11 years ago
Attachment #283812 - Flags: superreview?(mconnor)
Comment on attachment 283812 [details] [diff] [review]
smooth out status notifications

why is mCurrentPercentage a 64-bit integer?

you added some tabs to the cpp file

+    nsString statusMsg = nsDependentString(aMessage);
+    if (!statusMsg.Equals(mCurrentStatusMsg)) {

why not just:

  if (mCurrentStatusMsg.Equals(aMessage))

+      mStatusIsDirty = PR_TRUE;
+      mStatusMsg = statusMsg;

indentation here is 4 spaces, not two... also here:

+      MaybeSendStatus();
+      StartDelayTimer();


+    double progress = 0;
+    LL_L2D(progress, mCurProgress);

you don't have to use those macros anymore, you can just do:

  double progress = mCurProgress;

though it seems like you don't need the temporary variable then

+    if (percentage > (mCurrentPercentage + 3)) {

some sort of explanation about the significance of 3 would be nice
Attachment #283812 - Flags: review?(cbiesinger) → review+
if you could get jag to sr this that'd be great
(Assignee)

Comment 4

11 years ago
Created attachment 283855 [details] [diff] [review]
address biesi's comments

Additional review from jag is welcome, looking for mconnor's approval on the behavior changes.
Attachment #283812 - Attachment is obsolete: true
Attachment #283855 - Flags: superreview?(mconnor)
Attachment #283812 - Flags: superreview?(mconnor)
Comment on attachment 283855 [details] [diff] [review]
address biesi's comments

sr=mconnor, let's try this and see what happens with perf.
Attachment #283855 - Flags: superreview?(mconnor)
Attachment #283855 - Flags: superreview+
Attachment #283855 - Flags: approval1.9+
Comment on attachment 283855 [details] [diff] [review]
address biesi's comments

>Index: xpfe/browser/src/nsBrowserStatusFilter.cpp

>+nsBrowserStatusFilter::MaybeSendProgress() 

>+    // The progress meter only updates for increases greater than 3 percent

I think it would be a good idea to mention that this behavior is currently defined in progressmeter.xml, and that we're optimizing for the listener that updates the browser UI's progressmeter (this filter can used with other listeners, e.g. ones added by an extension via tabbrowser.addProgressListener).

Updated

11 years ago
Blocks: 397945
Keywords: perf
(Assignee)

Comment 7

11 years ago
I tried this patch with timeouts of 

360ms
320ms
280ms
240ms
200ms

and only 200ms didn't feel like an obvious slowdown. However, we are rendering this text using the high-quality path. We might be able to get away with 200 or 240 if we could write the status text faster, but XUL labels currently don't obey the CSS rules for this.
(In reply to comment #7)
> and only 200ms didn't feel like an obvious slowdown. However, we are rendering
> this text using the high-quality path. We might be able to get away with 200 or
> 240 if we could write the status text faster, but XUL labels currently don't
> obey the CSS rules for this.

Bug on that, blocking this one?

/be
(Assignee)

Comment 9

11 years ago
Created attachment 283884 [details]
before/after performance graph

This is from qm-mini-xp02, available on the MozillaTest tinderbox. The main boxes seem a little noisy, but showing improvements as well.
Yeah, I should be able to fix that fairly easily, but I don't know that you'll see much speedup. The text run cache should be doing a superb job here.
(Assignee)

Comment 11

11 years ago
(In reply to comment #10)
> Yeah, I should be able to fix that fairly easily, but I don't know that you'll
> see much speedup. The text run cache should be doing a superb job here.

After discussing with roc, this sounds pretty likely.

Thinking more about the status text updates, I think the problem with slower ones is that they get noticeably rhythmic. If we take this further, we should probably implement the progress meter in C++ at 120ms or so, and vary the frequency of status text updates.
Could you resolve this bug and file a new bug for any remaining work/issues?
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.