Closed
Bug 144533
Opened 22 years ago
Closed 22 years ago
Progress and Status change messages causing upto 30% overhead (in test case that exaggerates the problem, 10% on pageload tests).
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla1.1beta
People
(Reporter: ire0, Assigned: jag+mozilla)
References
Details
(Keywords: perf)
Attachments
(11 files, 6 obsolete files)
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+
asa
:
approval+
|
Details | Diff | Splinter Review |
18.23 KB,
patch
|
Details | Diff | Splinter Review |
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•22 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•22 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•22 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•22 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•22 years ago
|
||
I'm sorry, just who are "all nsIWebProgressListeners" ? This infrastructure is not needed for set of one.
Comment 7•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
Unzip this on your server or hardfile
Comment 13•22 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•22 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 15•22 years ago
|
||
Comment 16•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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.
Comment 29•22 years ago
|
||
Thanks. Added a comment to: http://bugzilla.mozilla.org/show_bug.cgi?id=136382
Comment 30•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
What is meant by the phrase 'other consumers'?
Comment 46•22 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•22 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•22 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•22 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
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.1beta
Assignee | ||
Comment 51•22 years ago
|
||
Attachment #88499 -
Attachment is obsolete: true
Assignee | ||
Comment 52•22 years ago
|
||
Comment 53•22 years ago
|
||
Comment on attachment 91791 [details] [diff] [review] update mac build fu (I hope) r=peterv
Attachment #91791 -
Flags: review+
Comment 54•22 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•22 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•22 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•22 years ago
|
||
I don't seem to be able to reproduce this problem.
Assignee | ||
Comment 58•22 years ago
|
||
There is a bug in this patch though in the enterTabbedMode code. I'm working on a fix.
Assignee | ||
Comment 60•22 years ago
|
||
jrgm, wanna see if this makes your browser happier for that site?
Assignee | ||
Updated•22 years ago
|
Attachment #91759 -
Attachment is obsolete: true
Attachment #91791 -
Attachment is obsolete: true
Comment 61•22 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 62•22 years ago
|
||
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 63•22 years ago
|
||
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•22 years ago
|
||
Attachment #93180 -
Attachment is obsolete: true
Assignee | ||
Comment 65•22 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•22 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•22 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•22 years ago
|
||
Attachment #93334 -
Attachment is obsolete: true
Assignee | ||
Comment 69•22 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 70•22 years ago
|
||
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•22 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•22 years ago
|
||
Apply the patch from that bug before applying this patch.
Assignee | ||
Comment 73•22 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•22 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•22 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
Comment 76•22 years ago
|
||
*** Bug 83840 has been marked as a duplicate of this bug. ***
Comment 77•22 years ago
|
||
reopening for mozilla1.1 consideration.
Reporter | ||
Comment 78•22 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•22 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•22 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: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 81•22 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•22 years ago
|
||
jag: ok sorry about reopen
Comment 83•22 years ago
|
||
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•22 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.
Comment 85•22 years ago
|
||
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.
Keywords: mozilla1.0.2 → mozilla1.0.2+
Comment 86•22 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.
Comment 87•22 years ago
|
||
Withdrawing 1.0 branch approval temporarily pending reviews and/or explanation of the differences between 1.0 and trunk patches.
Keywords: mozilla1.0.2+ → mozilla1.0.2
Assignee | ||
Comment 88•22 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•22 years ago
|
||
Thanks for the info. I'll give a look into that too. Sam
Assignee | ||
Comment 90•22 years ago
|
||
For completeness sake we'll also need to make sure the windows makefiles are updated.
Comment 91•22 years ago
|
||
If we can get this soon (sooner the better) we can get this into 1.0.2
Comment 92•22 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.
Comment 93•22 years ago
|
||
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•22 years ago
|
||
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•22 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•22 years ago
|
QA Contact: rakeshmishra → trix
Comment hidden (typo) |
Attachment #8957735 -
Attachment is obsolete: true
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•