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

RESOLVED FIXED in mozilla1.1beta

Status

()

defect
P3
normal
RESOLVED FIXED
17 years ago
3 months ago

People

(Reporter: ire0, Assigned: jag+mozilla)

Tracking

({perf})

Trunk
mozilla1.1beta
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 6 obsolete attachments)

225.67 KB, application/octet-stream
Details
2.81 KB, patch
Details | Diff | Splinter Review
1.52 KB, patch
Details | Diff | Splinter Review
1.79 KB, patch
Details | Diff | Splinter Review
3.99 KB, patch
Details | Diff | Splinter Review
943 bytes, patch
Details | Diff | Splinter Review
11.38 KB, patch
Details | Diff | Splinter Review
5.53 KB, text/plain
Details
35.57 KB, application/x-bzip2
Details
31.29 KB, patch
jag+mozilla
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
18.23 KB, patch
Details | Diff | Splinter Review
Reporter

Description

17 years ago
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

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

Comment 2

17 years ago
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

17 years ago
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

17 years ago
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

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

Comment 6

17 years ago
->rpotts. If events work is needed, please reassign
Assignee: joki → rpotts

Comment 7

17 years ago
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

17 years ago
"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

17 years ago
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

17 years ago
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
Reporter

Comment 11

17 years ago
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.

 





Reporter

Comment 12

17 years ago
Unzip this on your server or hardfile

Comment 13

17 years ago
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

17 years ago
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 16

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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?   
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.

Updated

17 years ago
Blocks: 71668

Comment 30

17 years ago
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

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

Comment 32

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

Comment 33

17 years ago
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
Reporter

Comment 34

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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?
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
(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.)
This information describes the profiles below.
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.
Attachment #88211 - Attachment description: .tar.bz2 → profiles of OnStateChange, OnStatus, and OnProgress (tar.bz2)

Comment 42

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
What is meant by the phrase 'other consumers'?

Comment 46

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
-> me
Assignee: rpotts → darin

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.1beta
Assignee

Comment 51

17 years ago
Attachment #88499 - Attachment is obsolete: true
Assignee

Comment 52

17 years ago
Posted patch update mac build fu (I hope) (obsolete) — Splinter Review
Comment on attachment 91791 [details] [diff] [review]
update mac build fu (I hope)

r=peterv
Attachment #91791 - Flags: review+

Comment 54

17 years ago
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.
Assignee

Comment 55

17 years ago
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

17 years ago
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.
Assignee

Comment 57

17 years ago
I don't seem to be able to reproduce this problem.
Assignee

Comment 58

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

Comment 59

17 years ago
-> jag
Assignee: darin → jaggernaut
Status: ASSIGNED → NEW
Assignee

Comment 60

17 years ago
jrgm, wanna see if this makes your browser happier for that site?
Assignee

Updated

17 years ago
Attachment #91759 - Attachment is obsolete: true
Attachment #91791 - Attachment is obsolete: true

Comment 61

17 years ago
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 on attachment 93180 [details] [diff] [review]
Fix tab not opening on first try when switching to tabbed mode

sr=bryner
Attachment #93180 - Flags: superreview+
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
Attachment #93180 - Flags: review+
Assignee

Comment 64

17 years ago
Posted patch Nits addressed. (obsolete) — Splinter Review
Attachment #93180 - Attachment is obsolete: true
Assignee

Comment 65

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

Carrying over r and sr
Attachment #93334 - Flags: superreview+
Attachment #93334 - Flags: review+
Assignee

Comment 66

17 years ago
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.
Assignee

Comment 67

17 years ago
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.
Assignee

Comment 68

17 years ago
Attachment #93334 - Attachment is obsolete: true
Assignee

Comment 69

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

Carrying over r=, sr= once more.
Attachment #93338 - Flags: superreview+
Attachment #93338 - Flags: review+
Comment on attachment 93338 [details] [diff] [review]
Address nits, for real this time.

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #93338 - Flags: approval+
Reporter

Comment 71

17 years ago
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
Assignee

Comment 72

17 years ago
Apply the patch from that bug before applying this patch.
Assignee

Comment 73

17 years ago
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee

Updated

17 years ago
Summary: Progress and Status change messages causing upto 30% overhead. → Progress and Status change messages causing upto 30% overhead (in test case that exaggerates the problem, 10% on pageload tests).
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?)
Reporter

Comment 75

17 years ago
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
*** Bug 83840 has been marked as a duplicate of this bug. ***

Comment 77

17 years ago
reopening for mozilla1.1 consideration.
Status: RESOLVED → REOPENED
Keywords: mozilla1.1
Resolution: FIXED → ---
Reporter

Comment 78

17 years ago
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

17 years ago
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.
Assignee

Comment 80

17 years ago
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.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 81

17 years ago
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

17 years ago
jag: ok sorry about reopen
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
Keywords: mozilla1.0.2

Comment 84

17 years ago
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.
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=rjesup@wgate.com for 1.0 branch checkin. 
Change mozilla1.0.2+ to fixed1.0.2 when checked in.

Comment 86

17 years ago
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. 
Withdrawing 1.0 branch approval temporarily pending reviews and/or explanation
of the differences between 1.0 and trunk patches.
Assignee

Comment 88

17 years ago
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

17 years ago
Thanks for the info. I'll give a look into that too.
Sam
Assignee

Comment 90

17 years ago
For completeness sake we'll also need to make sure the windows makefiles are
updated.
If we can get this soon (sooner the better) we can get this into 1.0.2

Comment 92

17 years ago
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.  
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.
Sam/jag/etc - the 1.0 branch patch is missing the nsBrowserStatusFilter.cpp
file; I assume it's the same as trunk.
Assignee

Comment 95

17 years ago
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.

Updated

17 years ago
QA Contact: rakeshmishra → trix
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.