144533 - Progress and Status change messages causing upto 30% overhead (in test case that exaggerates the problem, 10% on pageload tests).

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Ivan Eisen]

I modified nsDocLoader.cpp by commenting out FireonStateChange  in
nsDocLoaderImpl::doStartURLLoad and nsDocLoaderImpl::doStopURLLoad.Also, I
modified the nsDocLoaderImpl::FireOnStatusChange to do an immediate return;

These minor changes reduced load time of a document with 200 small gifs by 30%.
 This seems like an excessive amount of overhead for 
updating a progress bar.  The Status messages are later filtered out in the
nsBrowserStatusHandler.js routine (400 msec timer).

IE only shows the progres bar once and removes it after the document is loaded.
 Perhaps,a time element could be added to the code to
control the firing to every to every 400 msecs assuring that both first and last
 fire for a document.

Comment 1

[rpotts (gone)]

These times 'should' be decreased when the nsIWebProgress API changes land as
part of bug #46856.

Part of this patch allows callers to supply a 'mask' to
nsIWebProgress::AddProgressListener(...) indicating which kinds of notifications
need to be broadcast...

Comment 2

[Ivan Eisen]

How will the nsIWebProgress API changes reduce the number of progress events
that are triggered using FireonStateChange in nsDocLoaderImpl::doStartURLLoad
and nsDocLoaderImpl::doStopURLLoad ?  The progress events wind up in
nsBrowserStatusHandler.js. There seems to be 2 FireonStateChange calls triggered
for each gif that is in the document (200 gifs - 400 progress events).  In
addition, 400 other status events are being sent via
nsDocLoaderImpl::FireOnStatusChange.  I have a modified version of
nsBrowserStatusHandler.js that puts out on the status line after the document
finishes loading the total number received for each type of event instead of the
document load time. 



  

Comment 3

[Sam Emrick]

Can somebody point me to design docs explaining why all this low-level progress
reporting event passing infrastructire is.... why it is needed at all? Trying to
understand why all this is here. I'm working on code to just and only calc the
%complete in uriloader and periodicly post that. Seems sufficient. Why isn't it?  

Comment 4

[rpotts (gone)]

as i said before, the changes to nsIWebProgress::AddProgressListener(...) allow
listeners to 'pick and choose' which nsIWebProgressListener notifications it
needs to receive.

currently, all nsIWebProgressListeners receive ALL notifications (ie.
OnStateChange, OnProgress, OnStatus, ...)

by allowing individual listeners to 'opt in' to notifications, the total number
of calls should be greatly reduced.

Comment 5

[Sam Emrick]

I'm sorry, just who are "all nsIWebProgressListeners" ?
This infrastructure is not needed for set of one.

Comment 6

[saari (gone)]

->rpotts. If events work is needed, please reassign

Comment 7

[rpotts (gone)]

w.r.t nsBrowserStatusHandler.js... 

it is possible that something in the implementation is causing a large slow
down...    at various times, we have seen performance problems due to excessive
reflows/processing due to modifiying DOM attributes...

given the current algorithms for calculating progress, we need to receive
'request level' START/STOP notifications as well as Progress notifications.

this means that in your example (a document with 200 gif images) we MUST receive
402 notifications (START/STOP pairs for each gif image and the START/STOP of the
document).

we have been exploring whether or not it is necessary to create a 'lower-level
progress multiplexer' which would deal with the nsIWebProgressListener
notifications at a lower level (hopefully more effeciently)...  thus eliminating
much of the upcall traffic.

it would be interesting to see how much of the time is spent making the calls to
nsIWebProgressListener vs the amount of time spent processing within those
functions...

can you try putting early returns in the nsIWebProgressListener methods of
nsBrowserStatusHandler.js and see what the time difference is?

-- rick

Comment 8

[Sam Emrick]

"it is possible that something in the implementation is 
causing a large slow down...    at various times, we have 
seen performance problems due to excessive 
reflows/processing due to modifiying DOM attributes..."

That does not appear to be the problem.

"given the current algorithms for calculating progress, we 
need to receive 'request level' START/STOP notifications as 
well as Progress notifications. this means that in your 
example (a document with 200 gif images) we MUST receive
402 notifications (START/STOP pairs for each gif image and 
the START/STOP of the document)."

Yes. 1 Doc Start + 1 Doc Stop + 200 URL Start + URL 200 
Stop + 200 Progress Start/transfer + 200 Progress complete 
+ several handful of other status messages. I think 
'current algorithms for calculating progress' need rework.

"we have been exploring whether or not it is necessary to 
create a 'lower-levelprogress multiplexer' which would deal 
with the nsIWebProgressListener notifications at a lower 
level (hopefully more effeciently)...  thus eliminating
much of the upcall traffic."

Its simply not possible to reliably predict 'percent 
complete' part way through arbitrary stream. Theres a lot 
of code there now to manage the 'guestimation' of this 
quantity. Given this can ONLY be a guestimation, why not 
construct and accept a less-expensive guess.

Or even further, I think Ivan was gently noting that IE 
does almost nothing wrt progress and percent complete... 
so why does Mozilla? 

But assume not removed entirely, it needs be done cheaply.

Comment 9

[rpotts (gone)]

It's funny, because the progress algorithm we are using is derived from IE.  It
is extreamly simple and should not impose a large overhead.

There are two variations (based on the content type of the document).
1. text/html:
    Represent progress as the ratio of completed requests vs total requests.
    Once the document URL has finished loading, the total number of requests
    is known (excluding frames) and this ratio provides a fairly smooth approx.
    of progress.

2. all other content-types:
    Use the real progress data from the load.  This allows large full-page
    images (ftp downloads, etc) to provide meaningful progress feedback.

Do you have an alternative algorithm that you feel would be more efficient (but
still provide useful feedback)?

I find it *very* difficult to believe your assertion that it is the sheer number
of progress calls that is causing the overhead.  I cannot believe that 400
progress notifications while loading/rendering a page with 200 gif images
accounts for 30% of the time.

And once the nsIWebProgress API changes land, each listener will be able to
limit the notifications it receives...  This should significantly reduce the
number of notifications that the WebProgress implementation broadcasts.

As I said, it is quite possible that the listener implementation in
nsBrowserStatusHandler.js is causing performance problems, but I don't think
that that indicates that the entire underlying notification framework must be
reworked.

Can you provide some more detailed timing data?  If it turns out that our
browser-status-handler is a bottleneck, then we can certainly focus on reworking
it... but i'd like to understand exactly how it is the bottleneck before trying
it 'fix it'...
-- rick

Comment 10

[Sam Emrick]

Thanks Rick for the feedback. Problem is bouncing in and 
out of Javascript just to increment total/complete 
variables! I'd bet a week's salary that IE does not do 
this. I'd propose the total/complete counts should be 
maintained in uriloader, totals passed to javascript when 
needed. In addtion to URLstart/URLstop just to increment 
the request/complete variables, then also sending 
"Loading..." status message for each gif, and another to 
update the progress when each gif completes. Four bounces 
in and out of javascript for gif. Since the 200 gifs are 
painted in span of 2-3 seconds, most or all of the loading 
status messages are unproductive and shouldn't have been 
propogated. The gifs in Ivan's test are tiny, 
widget/button/control size things. The overhead for each 
notification, including entry/exit of javascript execution 
environment is non-trivial.  

The 'nsIWebProgress API changes' do NOT address the 
problem, I think, since they'll still require the bounces 
to increment variables, and send no-status status. The 
algorithm to count progress needs pulled out of 
BrowserStatusUpdate, I think.
Sam

Comment 11

[Ivan Eisen]

The "Loading..." status message for each gif is filtered out by a 400 msec timer
in the nsBrowserStatusHandler.js routine; however,  it would be more benefitial
to do it before the events are fired.  It is possible the OS/2 environment
in which we are testing makes the problem appear worst than on other systems.
I will provide the 200 duke testcase soon.

 

Comment 12

[Ivan Eisen]

Attachment: testcase used for performance test

Unzip this on your server or hardfile

Comment 13

[Sam Emrick]

Okay, I have a proof-concept full'o'debug prototype 
working, as I work to refine flow and towards patch'able 
change, ask here and see if this design is cause of any 
heartburn or grief. 

In uriloader\base\nsIWebProgressListener.idl a new flag
const unsigned long STATE_IS_PROGRESS    = 0x00100000;
then in uriloader\base\nsDocLoader.h some new counters
PRInt32 mURLstarts;
PRInt32 mURLstops;
then in uriloader\base\nsDocLoader.cpp
in nsDocLoaderImpl::doStartURLLoad and doStopURLLoad
bump those counters instead of FireOnStateChange(), 
instead, periodicly, 
int percent = (100 * mURLstops) / mURLstarts;
and 
FireOnStateChange(this, request, 
nsIWebProgressListener::STATE_IS_PROGRESS, percent);

Follow this? Concerns? Comments?
I'll keep working it towrard patch.

Comment 14

[Sam Emrick]

Attachment: nsSocketTransport.cpp patch

Part 1 of nsSocketTransport patch. Filter socket status messages at message
source, rather than sink, to save significant overhead in delivery unproductive
messages.

Forgive my inexperience with cvs diff. Doing cpp and h files in seperate patch
uploads.

Comment 15

[Sam Emrick]

Attachment: nsSocketTransport.h patch

Comment 16

[Darin Fisher]

sam: how much of a performance improvement do you see from these socket
transport changes?  modifications to that code need to be carefully analyzed
since there are multiple threads involved.

Comment 17

[Sam Emrick]

Darin, thanks for asking! 
This is data Ivan reported to our boss yesterday: 
"Modified the Network and Urlloader code to filter calls 
(Sam will explain changes).
Results of 200 Dukes test:
     OS/2 RC2 Before Fix --> 3.22 seconds
     OS/2 RC2 After fix  --> 2.31 seconds
     31% improvement
Comparison data:
     NT RC2 ---> 2.97 seconds
     NT IE 6.0 ---> 2.50 seconds"

Comment 18

[Sam Emrick]

Attachment: patch to nsFileTransport.cpp

Next part of OnStatus message filtering patches. The messages produced by
nsFileTransport are filtered and ignored by nsBrowserStatusHandler.js. So,
let's not produce them and save the overhead on re-load of cached elements.
(This patch independent nsSocketTransport patch)

Comment 19

[Sam Emrick]

Attachment: uriloader .cpp .h .idl patch for OnStatus filtering

Part 3a of OnStatus messages filtering patches. This is for uriloader, which
currently fires messages for each url element start and element complete. These
messages just do increment of variables in nsBrowserStatusHandler.js. The ratio
of these variables used as input to progress calcs. The patch moves counters
into uriloader, and creates a timer-based percent progress message from
uriloader to nsBrowserStatusHandler.js. Significant reduction in message
frequency reduces overhead of progress reporting but maintains progress
reporting mechanism for long documents.

(Getting better with cvs diff. All uriloader patches this file. The
nsBrowserStatusHandler.js patch to send seperate)

Comment 20

[Sam Emrick]

Attachment: nsBrowserStatusHandler.js patch to support STATE_IS_PROGRESS

Minimum patch to nsBrowserStatus to support previous patch to uriloader. If
this and previous patches accepted, then additional clean-up and streamlining
to nsBrowserStatus.js is possible.

Comment 21

[Darin Fisher]

sam: how can i learn more about the "200 Dukes test"??

blocking progress notifications from the file transport needs to be
programmatically controlled IMO.  some consumer might actually want those
notifications.  perhaps we need to add an attribute on nsIProgressEventSink that
the transport can query to learn what kinds of events the nsIProgressEventSink
impl actually cares about.  or, perhaps we need to split OnStatus and OnProgress
onto different interfaces.

another thing... maybe this performance penalty is due in large part to all the
xpcom/proxy code.  i wonder what would happen if we simply used raw plevents.

Comment 22

[Sam Emrick]

Darin, '200 dukes' is the zip that Ivan posted (1st 
attach). I can see maybe real local filesystem names would 
be useful somebody, but mostly these are mozilla file cache 
reads with obfusicated/hashed names... hard to think of 
usefulness that. 

Comment 23

[Darin Fisher]

the fact that your seeing the file transport being used to load cached data is
fine, but what about other uses of the file transport?  maybe other consumers
will want status notifications.

Comment 24

[Sam Emrick]

Darin, thanks for the additional comments. Not clear to me 
right this moment what other uses of nsFileTransport, if 
any, establish an OnStatus listener. You know any such? 
I'll investigate. Assume none exist, I'd propose comment 
out the 9 lines in nsFileTransport pending actual 
requirement for such status... or put them if yet another 
ifdef. Propose take actual existing performance benefit 
now, work potential future function requirements in future,

Anyway, the three parts, socket stream, file stream, and 
uriloader are independant, all three beneficial, but I'm 
not hard over all are required. Take what I can get... 
truly appreciate your input on the matter.

Comment 25

[Darin Fisher]

what would be very interesting to know is how much of a difference each separate
patch makes?  can you get performance results for each patch independent of the
other patches?  this will help us understand better what the tradeoffs of these
patches are.

Comment 26

[Sam Emrick]

Status
I've reviewed for dependencies on nsFileTransport OnStatus messages and can find
NONE, at least in current Mozilla code. I continue recommend not producing the
messages, at least in non-debug build.

Improvement in Ivan's testcase more or less half from nsSocketTransport changes
and half from uriloader/nsBrowserStatusHandler changes. 

On-going informal test of the recommended fixes have so far uncovered no
functional problems. We continue recommend fixes receive code review and move
toward entering mozilla public build. For the most part, I'm feeling/thinking
this problem idenfified, analyzed, and fixed... my focus will tend toward next
problem... 
Hope you'll well consider these fixes. 
Sam  

Comment 27

[Sam Emrick]

Additional finding by Ivan: 
in all.js 
pref("browser.display.show_image_placeholders", true);
is expensive.

Prior results of 200 Dukes test:
     OS/2 RC2 Before Fix --> 3.22 seconds
     OS/2 RC2 After fix  --> 2.31 seconds
     31% improvement 
with 
pref("browser.display.show_image_placeholders", false);
     OS/2 RC2 After fix  --> 1.88 seconds
(43%+ improved, now we're gettin there!)

the 'problem' with current code placeholders==false is also 
lose broken-image icons too, which have higher utility than 
placeholder icons. We may submit bug to change pref to 
false and 1-2 line code change to make brokens still show 
when false. Comments on doing such?   

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you want to do that, please file it as a separate bug and cc: the people who
have been involved in discussing that behavior in the past.

Comment 29

[Sam Emrick]

Thanks. Added a comment to:
http://bugzilla.mozilla.org/show_bug.cgi?id=136382

Comment 30

[rpotts (gone)]

were your test conducted on a debug or release build of mozilla?

i tried running your test case on a release build (tip) and got load times of
between 0.45-0.5 seconds...  which made it impossible to determine if there was
any difference with or without the status notifications...

were you running the testcase, locally or from a server? also, what was the
machine configuration that you used?

(my test machine was a P3/850mhz, 384Mb RAM running Win2K)

thanks,
-- rick

Comment 31

[Sam Emrick]

333MHz clients using rc2 level source non-debug optimized 
build over 100mbps ethernet to 4-way 700 MHz html server. 
Of course, filtering socket status is useful only in case 
input stream is socket, not disk file.  

Comment 32

[Ivan Eisen]

I tested this fix against a collection of simulated web sites which are on my
server (IBM, Formula1, Espn , CNN, etc.) and found with this fix  RC2
under OS/2 is just as fast as IE6. I don't see a down side for implementing
this fix.  It would take only minor modifications to allow the blocking of
status notifications from the file transport to be optional.  I am not clear why
you would want these notifications from the file transport (other then for
debugging purposes). 

Comment 33

[Ivan Eisen]

Sam and I are very interested in getting these patches reviewed and getting
this bug into the FIXED state.  It is critical for good performance for any
web site that has lots of objects. We are especially interested if there is
any areas where this fix could cause problems.  We could modify the fix to
to turned on and off by a preference in all.js if you feel that the status
messages may be someday needed for debugging. This preposed fix is a simple
method for removing lots of overhead from our processing.

Thanks,
Ivan

Comment 34

[Ivan Eisen]

Linux measurements for the proposed status messaging fix.  I tested Mozilla 1.1A
for june 17th with and without our fix.  I used the 200 duke testcase which
consists of 200 different small gifs that appear on the screen.
I ran on a 666 MHz 256mb Pentium 3 with Redhat 7.2.
I ran this test multiple times clearing the caches.

Results:
Original 6/17 1.1A --->  averaged 1.33 seconds
Fixed version      --->  averaged  .94 seconds

 That is approximately a 30% improvement which is also true in OS/2.  

We need this improvement to be competitive with IE6 for most websites.
       

Comment 35

[Darin Fisher]

ivan: which patch(es) were you testing?  can you run your tests with each patch
individually so we can know which one(s) makes the most difference?  thx!

Comment 36

[Sam Emrick]

Darin, fix parts include, network-streams, file-streams, 
and progress messages. Ivan's testcase is network, so the 
file-streams improvement is mostly not benefit. When uri's 
become cached, then the file streams fix pertinent. The 
network-streams part of the fix involves two messages per 
object savings. The progress messages part of the fix also 
involves two messages per object and simplification of the 
progress calculations in javascript. For Ivan's specific 
testcase the savings is probably about 50%/50% for the 
network streams fix and the progress fix. Your mileage may 
vary... but... all parts of this are significant 
goodness... we don't understand why any part of the fix is 
not goodness... its small number lines of code and just 
trims function that is currently unused.

Comment 37

[Darin Fisher]

agreed, it sounds like you have good patch here, but i think it's worth
understanding where the costs are.  it'd be nice to know how much of a perf win
each individual patch gives and why.  that way we can apply what we learn to
other parts of the mozilla codebase.  maybe we'll learn that using the
xpcom/proxy code to proxy status events is costly.  maybe we'd then know to trim
our use of xpcom/proxy code elsewhere.  but without doing the analysis we'll
never know.  i'm just asking you guys to help us understand the problem better.
 sure, it's nice to just fix the perf bug, but i think we can learn something
here as well.  make sense?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: merged patch of all above patches, updated to trunk

I've been noticing this problem in profiles that I've been looking at recently.
 I'll attach some profiles in a bit.  In any case, this patch is a merged patch
of all the patches above, updated to the trunk.

I ran some of jrgm's performance tests on this patch, with an optimized build,
on a 1.5GHz Win2KPro machine, and saw the following results:

without patch

Test id: 3D0F96003C
Avg. Median : 474 msec		Minimum     : 157 msec
Average     : 491 msec		Maximum     : 1518 msec

Test id: 3D0F982DD5
Avg. Median : 461 msec		Minimum     : 147 msec
Average     : 457 msec		Maximum     : 1322 msec

Test id: 3D0F998020
Avg. Median : 466 msec		Minimum     : 152 msec
Average     : 464 msec		Maximum     : 1348 msec



with patch

Test id: 3D0FAC675F
Avg. Median : 410 msec		Minimum     : 142 msec
Average     : 409 msec		Maximum     : 1198 msec

Test id: 3D0FADC547
Avg. Median : 410 msec		Minimum     : 146 msec
Average     : 409 msec		Maximum     : 1218 msec

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(One thing I wonder, though, is whether this patch could mess with the test
numbers without actually speeding things up.  However, the profile data suggests
that this speedup is realistic.)

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: information on profiles attached below

This information describes the profiles below.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: profiles of OnStateChange, OnStatus, and OnProgress (tar.bz2)

This is 144533profiles.tar.bz2, which contains three files (which are bigger
than the bugzilla attachment size limit, but which compress very well):

11-OnStateChange.html :  This profile shows the time spent within
nsDocLoaderImpl::FireOnStateChange.  Note that it excludes, among other
things, the time spent within DocumentViewerImpl::LoadComplete.  See 
attachment 88207 [details].  This is 7.2% of the time in the page loading profile
(main thread time doing things other than poll).

22-OnStatus.html : Likewise, but nsDocLoaderImpl::OnStatus.  This is
2.8% of the page loading profile.

26-OnProgress.html : Likewise, but nsDocLoaderImpl::OnProgress.  This is
0.4% of the page loading profile.

Comment 42

[Darin Fisher]

so it seems to me that one good way to solve the OnStateChange part of this
problem might be to introduce a progress like state as the current patch does,
but instead of commenting out the calls to FireOnStateChange in onStartURLLoad
and onStopURLLoad we could just use the notify mask to block STATE_IS_REQUEST
messages from being sent to nsBrowserStatusHandler.js.

Comment 43

[Darin Fisher]

actually, i've thought about this some more, and why can't we simply push
nsBrowserStatusHandler's logic that turns OnStart/OnStop notifications into
OnProgressChange notifications down into nsDocLoader?  that is,
nsIWebProgress::addProgressListener could specify that it wants nsDocLoader to
do this.  the default could remain as it is today, but navigator.js (and
fastnav.js) could request that this algorithm be employed.

Comment 44

[Darin Fisher]

jag mentioned that it probably isn't such a great idea to add special knowledge
of this algorithm in nsDocLoader because other consumers likewise have special
algorithms.  ic his point.

then my next thought is to create a C++ nsIWebProgress/nsIWebProgressListener
implementation that can sit between nsBrowserStatusHandler and nsDocLoader.  it
would implement the logic that turns OnStart/OnStop notifications into
OnProgressChange notifications on behalf of nsBrowserStatusHandler.

this is probably just as good as pushing the impl down into nsDocLoader and a
bit cleaner/simpler.

Comment 45

[Sam Emrick]

What is meant by the phrase 'other consumers'?

Comment 46

[Darin Fisher]

sam: there are many other nsIWebProgressListener implementations throughout the
mozilla code base... these are therefore "other consumers of web progress
events."  more specifically, i was thinking of the download manager.  it has yet
another optimization algorithm that it uses for its status display.

the patches that you guys developed have revealed some really nasty performance
problems in mozilla, but they aren't anywhere near acceptable because they break
the docloader API.  while the breakage may be fine for the browser window, it
probably isn't fine for many of the other nsIWebProgressListener implementations
used by other mozilla components.  moreover, the real performance problem (as
indicated by dbaron's jprof data) is the invocation of javascript code to handle
the nsIWebProgressListener events.  so, i'm thinking that all we really need to
do is move some of the nsBrowserStatusHandler implementation (specically the
parts that filter out certain events and delay notifications) from javascript to
C++.  i'm working on a patch that does exactly this.

Comment 47

[Sam Emrick]

Darin, thanks for the interest, feedback, and words of 
encouragement! Some additional comments. On 'consumers', I 
just needed clarification this meant Mozilla code-base 
nsIWebProgress listeners, not things unknown to me 
(derivative browsers not in mozilla code tree, or 
whatever). As to 'many nsIWebProgress implementations', 
ummm... I count six, I think.

Iam NOT trying to defend my fix / patch. Its served its 
purpose to demonstrate problem and quatify opportunity.
Having done that... now we can move on...

For Download Manager, please give a look at 
nsDownloadProgressListener.js. Notice its does the same 
sort of time-based filtering/discarding of progress status  
messages as nsBrowserStatusHandler.js. Both could use a 
more simple/faster progress notification from DocLoader 
(docloader itself calcs percent complete and periodicly 
sends 'percent complete' message).

Also in nsDownloadProgressListener.js, this sad bit of 
code:

onLocationChange: function(aWebProgress, aRequest, 
aLocation, aDownload)
    {
    },
onStatusChange: function(aWebProgress, aRequest, aStatus, 
aMessage, aDownload)
    {
    },
onSecurityChange: function(aWebProgress, aRequest, state, 
aDownload)
    {
    },
 
So...bouncing in-out of javascript to do exactly nothing..

Several the other nsIWebProgress listeners code is sadly 
similar, quick example, see:
http://lxr.mozilla.org/seamonkey/source/editor/composer/src
/nsEditingSession.cp#373
Likewise, composer simply discards almost all 
nsIWebProgress messages.  

So, I think I am saying, nsBrowserStatusHandler shows us 
one instance of what is a pervuasive problem, excessive 
generation nsIWebProgress messages and unneeded, 
non-productive handling of the messages by listeners. 

For browsing, we see 10% ('average' testcase) to 30% 
('problem-focused' testcase) of the total time is spent in 
generation and handing unproductive messages. Based on code 
inspection it would seem there may be similar problem and 
opportunity in Download Manager performance and the other 
nsIWebProgress listener implementations too. 

I'm thinking, my opinion is, if all users/implementations 
of nsIWebProgress interface are needing to do bad things to 
use it, then need take hard look at basic changes to the 
interface.   

Again, appreciate your interest. Thanks!

  






      
         

Comment 48

[Darin Fisher]

Attachment: patch to add a C++ component between nsBrowserStatusHandler.js and nsDocShell -- incomplete

this is an incomplete patch that i've been working on that basically implements
the extra C++ component i mentioned above.  it isn't complete by any means, and
i still have to address the tab browser issue.	i'm just posting it here for
feedback, etc.

Comment 49

[Cathleen]

Some page-load test result:
----------------------------------------

machine: win98, 200mHz, 112MB RAM
build  : trunk pull on 6/20/02

trunk 6/20                    : 2512 ms
w/ patch 1 (attachment 88200 [details] [diff] [review]) : 2370 ms  
w/ patch 2 (attachment 88499 [details] [diff] [review]) : 2389 ms

Comment 50

[Darin Fisher]

-> me

Comment 51

[jag (Peter Annema)]

Attachment: Patch that better integrates with tabbrowser

Comment 52

[jag (Peter Annema)]

Attachment: update mac build fu (I hope)

Comment 53

[Peter Van der Beken [:peterv]]

Comment on attachment 91791 [details] [diff] [review]
update mac build fu (I hope)

r=peterv

Comment 54

[John Morrison]

I did a couple of tests with attachment 91759 [details] [diff] [review], win2k/500MHz/128MB, one test 
with no tabs, and the second test with three tabs open, left and right tabs
set to mozilla.org, and middle tab running pageload test. 

I measured a gain of about 5% for the no-tabs case, and about 7% for the 
three-tabs case.

Comment 55

[jag (Peter Annema)]

Thanks John. My measurements show a 3% win when no tabs open on a 933Mhz 256MB
RAM W2k machine (I guess the "slowness" of JS is less of a factor with CPU speed
going up).

Comment 56

[John Morrison]

In my build with the last patch, I am regularly (always?) seeing a problem 
with http://www.news.com/. That URL responds with a 302 Moved Permanently
redirect to http://news.com.com/. At the end of (what should be) loading that
page, the throbber remains animated, and the progress meter is at ~95% and 
stuck. This is in a win2k build, optimized, trunk.

Comment 57

[jag (Peter Annema)]

I don't seem to be able to reproduce this problem.

Comment 58

[jag (Peter Annema)]

There is a bug in this patch though in the enterTabbedMode code. I'm working on
a fix.

Comment 59

[Darin Fisher]

-> jag

Comment 60

[jag (Peter Annema)]

Attachment: Fix tab not opening on first try when switching to tabbed mode

jrgm, wanna see if this makes your browser happier for that site?

Comment 61

[John Morrison]

With the latest patch applied, in a current trunk build, I'm not seeing the 
problem that I noted before. I just keep using this and see if anything comes
up.

Comment 62

[Brian Ryner (not reading)]

Comment on attachment 93180 [details] [diff] [review]
Fix tab not opening on first try when switching to tabbed mode

sr=bryner

Comment 63

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 93180 [details] [diff] [review]
Fix tab not opening on first try when switching to tabbed mode

Nice.  r=caillon with just a couple of minor nits:

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

In nsBrowserStatusFilter::OnStateChange()

>+            mTotalRequests++;
...
>+            mFinishedRequests++;

  ++mTotalRequests;
  ++mFinishedRequests;


In nsBrowserStatusFilter::StartDelayTimer()

>+    NS_ASSERTION(DelayInEffect() == PR_FALSE, "delay should not be in effect");


  NS_ASSERTION(!DelayInEffect(), ...); 


>+void
>+nsBrowserStatusFilter::TimeoutHandler(nsITimer *aTimer, void *aClosure)
>+{
>+    nsBrowserStatusFilter *self = (nsBrowserStatusFilter *) aClosure;


Use NS_STATIC_CAST, please.


Index: xpfe/global/resources/content/bindings/tabbrowser.xml
===================================================================

>       <destructor>
>         <![CDATA[
>+          for (var i = 0; i < this.mTabListeners.length; i++) {

  ++i

Comment 64

[jag (Peter Annema)]

Attachment: Nits addressed.

Comment 65

[jag (Peter Annema)]

Comment on attachment 93334 [details] [diff] [review]
Nits addressed.

Carrying over r and sr

Comment 66

[jag (Peter Annema)]

Comment on attachment 93334 [details] [diff] [review]
Nits addressed.

Looks like I didn't save after changing the post-inc to pre-inc in
tabbrowser.xml. Will change that before checking in, I promise.

Comment 67

[jag (Peter Annema)]

Comment on attachment 93334 [details] [diff] [review]
Nits addressed.

Scratch that. I attached the same diff. Not sure what happened here, I could
swear I answered yes when vi asked if it should overwrite.

Comment 68

[jag (Peter Annema)]

Attachment: Address nits, for real this time.

Comment 69

[jag (Peter Annema)]

Comment on attachment 93338 [details] [diff] [review]
Address nits, for real this time.

Carrying over r=, sr= once more.

Comment 70

[Asa Dotzler [:asa]]

Comment on attachment 93338 [details] [diff] [review]
Address nits, for real this time.

a=asa (on behalf of drivers) for checkin to 1.1

Comment 71

[Ivan Eisen]

I am having some problems with this patch.  I want to drop this patch on the 1.0.1
code base.  When I try this I am getting an error for GetIsLoadingDocument in
nsBrowserStatusFilter.cpp.  This may be related to the fix for Bug 133250 that
is not part of 1.0.1.  I really want to drop this fix on the 1.0.1 code base and
test it.  Could you please provide some guidance.
Thanks,
Ivan

Comment 72

[jag (Peter Annema)]

Apply the patch from that bug before applying this patch.

Comment 73

[jag (Peter Annema)]

Checked in.

Comment 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Poking at this in the debugger and in profiles, it looks like some of the
notifications are going through the filter, but some are going directly from the
docloader to the nsBrowserStatusHandler.  The problem seems particularly bad for
OnStateChange, although it also looks like it's present for OnStatus.

Could this patch not be hooking something up to the filter?  (Would it be
cleaner to hide the filter inside of browser.xml and it's addProgressListener
method?)

Comment 75

[Ivan Eisen]

We would really like this fix for 1.0.1 ;  however, 2 previous (non 1.0.1)
patches are required. The dependent patches are for bug 133250 and bug 144533. 
We can't use those patches because they change our NLS translations requirements.  

I tries patching nsBrowserFilter.cpp without the GetIsLoadingDocument() function
but the resulting build would not run correctly (the buttons on the toolbar were
not being enabled and disabled correctly).  What do I need to do to get this fix
onto 1.0.1.  However, the nsBrowserStatusHandler.js is modified quite a bit by
the previous 2 fixes.  This fix really helps our performance on OS/2 for most
websites.

Thanks,
Ivan

Comment 76

[Boris Zbarsky [:bzbarsky]]

*** Bug 83840 has been marked as a duplicate of this bug. ***

Comment 77

[Chris McAfee]

reopening for mozilla1.1 consideration.

Comment 78

[Ivan Eisen]

I think this bug currently depends on the patch for bug 133250.  Does opening
this bug up for consideration for 1.1 also open up bug 133250?

Ivan

Comment 79

[Chris McAfee]

Just a followup to my 1.1 vote, this fix on the trunk had 5-7% time
improvement on Tp, also some wins on the other tests.  I think
we should take this change for the branch if at all possible,
5-7% is a big deal.

Comment 80

[jag (Peter Annema)]

It's a nice win, but Mozilla.org is not going to take this for 1.1. Btw, if you
want to nominate a bug for a certain milestone, use the keyword fields instead
of reopening the bug (which makes it look like it hasn't been fixed yet, or has
broken something, on the trunk).

Ivan, I have no simple answer for you on how to apply this patch to your 1.0.1
tree. A lot of changes have been made between 1.0.1 and now that allow this
patch to work, if you want it in 1.0.1 you'll have to find out which changes,
and backport them.

Comment 81

[Sam Emrick]

I've backported the patch to MOZILLA_1_0_BRANCH, tested it, and have 
recommended to M.Kaply that we include this fix in our next ibm branded 
os/2 web browser release. If somebody else wants the 1.01 patch, or if 
questions or concerns about this, then please email me. Thanks
Sam  

Comment 82

[Chris McAfee]

jag: ok sorry about reopen

Comment 83

[Brendan Eich [:brendan]]

Thanks to the IBM guys for their work on this one.  Sam, could you please attach
a 1.0-branch-ready version of the patch?  Drivers are interested in getting it
in for 1.0.2, if everyone else agrees.  Thanks,

/be

Comment 84

[Sam Emrick]

Attachment: 1.0-branch version of the fix

Here is the changed-files part of the 1.0-branch patch. Something about 
my 'permissions' doesn't make new-file diff's work right for me. The new files
mozilla/xpfe/browser/src/nsBrowserStatusFilter.cpp and 
mozilla/xpfe/browser/src/nsBrowserStatusFilter.h are identical to the 
1.2-branch version of the files.

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

Sam:

Reviewing the differences between the 1.0 branch version and trunk:

The macbuild/mozBrowser.xml changes aren't in the 1.0 branch patch; is this correct?

In nsBrowserStatusHandler.js, there are a bunch of significant differences (it
isn't clear why) with more changes in the 1.0 branch patch than the trunk
(perhaps incorporating things that were already in trunk, or from other bugs
that were prerequisites of doing this patch?).

If those are correct for 1.0 then a=[e-mail] for 1.0 branch checkin. 
Change mozilla1.0.2+ to fixed1.0.2 when checked in.

Comment 86

[Sam Emrick]

Mea culpa, when I was doing this patch my own purpose I wasn't worried 
about mac builds. I don't know the answer about macbuild changes. 
 
Yes, the additional nsBrowserStatusHandler.js changes are to 
incorporate the essential prereq code patches without picking up 
message string changes, included in the prereqs, that would need 
translation. Needing just parts of the prereqs patches, the easiest 
thing was to include the parts that were needed in this one patch. 

Comment 87

[Randell Jesup [:jesup] (needinfo me)]

Withdrawing 1.0 branch approval temporarily pending reviews and/or explanation
of the differences between 1.0 and trunk patches.

Comment 88

[jag (Peter Annema)]

There are some changes in that patch which might not quite be necessary but
won't hurt either (in fact, they'll make live a little better).

There's a problem though: bug 165867 (topcrash)

Basically there's a race condition between removing the listener and checking it
for null and notifying it. Until that is fixed, this shouldn't go in on the branch.

Comment 89

[Sam Emrick]

Thanks for the info. I'll give a look into that too.
Sam

Comment 90

[jag (Peter Annema)]

For completeness sake we'll also need to make sure the windows makefiles are
updated.

Comment 91

[Randell Jesup [:jesup] (needinfo me)]

If we can get this soon (sooner the better) we can get this into 1.0.2

Comment 92

[Sam Emrick]

Randell, what bothers me the most is the reported topcrash 
http://bugzilla.mozilla.org/show_bug.cgi?id=165867 in the the trunk 
version. I posted a suggested patch in that bug but got no feedback.
The odds and ends (fixing upm acbuild/mozBrowser.xml and makefile.win 
are easy to do, but not IMO worthwhile to spend time on the 1.02 patch 
if we're not sure the trunk version of the patch is robust. Please give 
me more explicit advice how to best proceed. Thanks.  

Comment 93

[Randell Jesup [:jesup] (needinfo me)]

Since there's a fix for bug 165867, let's get this (including that fix) into
branch for 1.0.2.

Sam, please get jag and/or darin to review and get a superreview.

Comment 94

[Randell Jesup [:jesup] (needinfo me)]

Sam/jag/etc - the 1.0 branch patch is missing the nsBrowserStatusFilter.cpp
file; I assume it's the same as trunk.

Comment 95

[jag (Peter Annema)]

I'm keeping an eye on talkback reports. If we don't get anything for about a
week I'm willing to assume that my patch fixed the crasher and we can go ahead
with this for the branch.

Comment 96

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/telemetry-batch-view/pull/394