Closed Bug 398824 Opened 17 years ago Closed 17 years ago

smooth out status notifications

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Assigned: sayrer)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch smooth out status notifications (obsolete) — Splinter Review
Assignee: nobody → sayrer
Status: NEW → ASSIGNED
Attachment #283812 - Flags: review?(cbiesinger)
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
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).
Blocks: 397945
Keywords: perf
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
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.
(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?
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.