Closed
Bug 273578
Opened 18 years ago
Closed 17 years ago
XMLHttpRequest with async = false takes 100% CPU until request finished
Categories
(Core :: XML, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.8rc1
People
(Reporter: matiyam, Assigned: darin.moz)
References
Details
(Keywords: fixed1.8, perf, testcase)
Attachments
(4 files, 2 obsolete files)
903 bytes,
patch
|
Details | Diff | Splinter Review | |
3.61 KB,
application/x-gzip
|
Details | |
873 bytes,
patch
|
jst
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616 Hi! When I use a XMLHttpRequest object to fetch a file with async = false, both Firefox and Mozilla take 100% of the CPU until the request have been completed. If I ask for a long .xml file, my machine almost freezes until the file has been downloaded. I have a small cgi script that just writes a small xml text, but with a delay of 10 seconds. If the request is made with async=true, everything works flawlessly (Mozilla just waits until the script have been downloaded). But if the request is made with async=false, Mozilla takes 100% of the CPU until the script is downloaded. I guess it has something to do with the interaction between the parent process and the thread that makes the new petition. Probably the parent is using some "while" without using some blocking mechanism to wait without using CPU. I attach a small cgi that sleeps for a while, so Mozilla has to wait for a while. Test cgi script: xml.cgi ------------------ #!/bin/bash echo "Content-type: text/xml" echo sleep 10 echo "<?xml version='1.0' encoding='ISO-8859-1'?><root></root>" ------------------ Test javascript code: ---------------------- function load_nonasyncronous(url) { var o = XMLHttpRequest(); o.open( "GET", url, false ); o.send(null); alert("result: " + o.responseText); } function load_asyncronous(url) { var o = XMLHttpRequest(); o.onreadystatechange = function() { if (o.readyState == 4) alert("result: " + o.responseText); o.open( "GET", url, true ); o.send(null); } ----------------------- Thanks for such a great browser (and such a huge effort). Reproducible: Always Steps to Reproduce: 1.Write some js that fetches a xml from a CGI with some delay using XMLHttpRequest and async = false 2. See CPU usage until the xml is fetched 3. Actual Results: The CPU usage of Mozilla raises up to 100% Expected Results: Mozilla should have waited for the file without using CPU
*** This bug has been marked as a duplicate of 190313 ***
Status: UNCONFIRMED → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE
In Comment 1 Heikki marks this bug as a duplicate of Bug 190313 (XMLExtras syncloading blocks UI (syncloadservice)), but i don't think this is really a duplicate. It's not that the UI gets blocked (that could be a correct behaviour). The problem is that the CPU usage raises to 100%. There's another bug like this one (Bug 240678 (Mozilla 1.7b (and Firefox 0.8) is busy waiting on synchronous xmlhttprequest)), but that is already marked as a duplicate of 190313. Could someone verify this?
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
(In reply to comment #2) > In Comment 1 Heikki marks this bug as a duplicate of Bug 190313 (XMLExtras > syncloading blocks UI (syncloadservice)), but i don't think this is really a > duplicate. It's not that the UI gets blocked (that could be a correct behaviour). > The problem is that the CPU usage raises to 100%. The UI should never be blocked by JS running on a page. The reason it is blocked is because all the XML processing happens in a tight loop that does not yield to the UI. Incidentally, that also manifests as 100% CPU. I still believe this bug is duplicate of bug 190313. The only case where this would not be a duplicate is if you can use the UI while the CPU is pegged at 100% in the Mozilla process.
I have tested the patch with the nightly codes (2005/01/21) on my sparc machine, it is one time faster than before
(In reply to comment #4) > Created an attachment (id=172074) [edit] > Use WaitForEvent() instead of ProcessPendingEvent() > > I have tested the patch with the nightly codes (2005/01/21) on my sparc > machine, it is one time faster than before I believe WaitForEvent() works as well as ProcessPendingEvent(), but AFAIK it has the same problem - you hit 100% CPU locking the UI until request finishes. It *may* be slightly more efficient, but it is not the real solution IMO. Look at bug 190313 for more ideas (I still maintain these are dupes).
I think the root cause is in the following codes: // If we're synchronous, spin an event loop here and wait if (!(mState & XML_HTTP_REQUEST_ASYNC)) { while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { modalEventQueue->ProcessPendingEvents(); } when in sync mode, this thread will in a busy loop, which will waste a lot of CPU time. I think the solution is to block current thread until another thread posts an event in the modalEventQueue.
(In reply to comment #7) > I think the root cause is in the following codes: Yes (but I think I have explained this earlier in the dupe or some other bug) this code was put in so sync loading would also work in embedded builds. Before, sync loading was done in such a way that it did not block the UI (or cause 100% CPU) but it only worked in the normal Mozilla builds, not in the embedded case. As far as I know, the main question which still hasn't been resolved is what kind of technology to use so that it will work without blocking the UI and still work in embedded and normal case. The embedded case is problematic because there Mozilla does not own the event loop. > when in sync mode, this thread will in a busy loop, which will waste a lot of > CPU time. I think the solution is to block current thread until another thread > posts an event in the modalEventQueue. No. The current thread is the UI, so you must not block it. But maybe it would be possible to push the whole XMLHttpRequest into the network thread or something.
Comment 10•18 years ago
|
||
(In reply to comment #5) > Can anyone help me to verify if the patch works for you? Thanks Hello the patch https://bugzilla.mozilla.org/attachment.cgi?id=172074 works for me (i apply the patch manually). The opensi application (http://www.opensi.org/) does not work on windows without this patch (without the patch firefox or mozilla : 100% CPU / with the patch and mozilla : % of use of CPU is OK). In french : Description of the problem with opensi application without the patch : http://forums.opensi.org/viewtopic.php?t=126&sid=d6e823a4e2a3765fc543726d8309fe06
Attachment #172403 -
Attachment is obsolete: true
Comment 11•18 years ago
|
||
Attachment #175905 -
Attachment is obsolete: true
Comment 12•18 years ago
|
||
Comment 13•18 years ago
|
||
I don't think we can move XMLHttpRequest related codes to somewhere else. Because these javascript codes run in javascript interpreter, which runs in the current thread(UI thread). I don't know if there is some way to let a piece of Javascript codes run in another thread.
![]() |
||
Comment 14•18 years ago
|
||
Boying, what do you mean, exactly? The code that actually does the sync request is in C++, not in JS....
Comment 15•18 years ago
|
||
In the following snippet: fetcher.open("POST",getServletURL(),false); fetcher.setRequestHeader("Content-type","text/plain; charset=UTF-8"); try { fetcher.send(infotosent); }catch (e) { window.alert("send error:"+e); } if (typeof fetcher.responseText=="string"&&fetcher.responseText.length>0) { window.alert("time taken:" + (end-start)+"ms\nServerRespons:\n"+fetcher.responseText); } where fetcher is an XMLHttpRequest object and previous codes are trigged by a button embedding in a HTML page. All above codes will be run in UI thread. If we move the implementation of send() method into an other thread. The UI thread still should be blocked at the last "if" statment in sync mode because fetcher.responseText contains servers response.
![]() |
||
Comment 16•18 years ago
|
||
Oh, I see. That was in response to comment 8.... Yeah, I think we want to keep this on the UI thread. If we can use the syncload service, we should. If we can't, we should probably make the minor tweaks needed so we _can_ use it and use it.
Assignee | ||
Comment 17•18 years ago
|
||
The sync load service works by calling WaitForEvent. That method calls PR_Wait on a monitor. That means that if called on the UI thread, it will lock up the UI until some new PLEvent is added to the queue. This means that UI events will starve. It is definitely suboptimal. ProcessingPendingEvents works by allowing all PLEvents and native events a chance to run, but it does not block waiting for either. What we really need is a way to say: block for PLEvents or native events. I think the event queue system should be revised to somehow communicate better with the appshell to make this possible. confirming bug
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 18•17 years ago
|
||
In my JS-based application I see complete starvation and hang from time to time when using Mozilla or Firefox builds with SVG enabled (trunk builds from mid-March 2005 on Win2K): the HTTP request is sent but nothing more happens, except 100% cpu load. What makes the thing even more worse is, say have Firefox running and blocking, and now start a Mozilla instance running the same JS application and using XMLHttpRequest, it blocks and hangs the same as the other Firefox (!) instance. Note that the server is responding and has no problems, it is just that both FF and Moz block for no appearant reason. Only after several minutes of waiting it seems that some semaphore gets released, so FF and Moz work again.
Comment 19•17 years ago
|
||
bz, darin, anyone: who should own this bug? /be
![]() |
||
Comment 20•17 years ago
|
||
Someone who understands appshell, preferably... It sounds like we want to go back out to the OS-level event loop and wait on OS events, but with an event queue pushed and without actually calling "return" from the function we're in.
Assignee | ||
Comment 21•17 years ago
|
||
So, in other words, we need the code from OpenWindowJS ;-) I can add this to my queue, but I'm already overburdened for Firefox 1.1. I would greatly appreciate some help. Jonas? ;-)
Assignee: xml → darin
Severity: normal → major
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta2
Yeah, I'd be interested in working on this since I think it's an important bug. I'm not sure how much time i'll have available just yet though. Should know more in a week or two.
Comment 23•17 years ago
|
||
Is it dupe of bug 190313?
no
Assignee | ||
Comment 25•17 years ago
|
||
This is related to bug 309424
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.8beta2 → mozilla1.8beta5
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8rc1?
Comment 26•17 years ago
|
||
Jonas, have you had any cycles to investigate here? Is there a possibility of a low-risk fix?
I havn't investigated yet, but i really doubt there's anything lowrisk to go on the branch.
Comment 28•17 years ago
|
||
it's too late for anything but low-risk fixes. the time to take a fix for something like this was months ago.
Flags: blocking1.8rc1? → blocking1.8rc1-
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.8beta5 → mozilla1.9alpha
Comment 29•17 years ago
|
||
(In reply to comment #27) > I havn't investigated yet, but i really doubt there's anything lowrisk to go on the branch. The fix I tried was a oneliner: Add a PR_SLEEP with 10 miliseconds in the loop. while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { PR_Sleep( PR_MillisecondsToInterval(10)); modalEventQueue->ProcessPendingEvents(); } Can you add this as a low risk fix for the 100% cpu and use the other defects for the long term solutions as described by defect 190313 ?
Comment 30•17 years ago
|
||
Attachment #199529 -
Flags: review?(peterv)
Assignee | ||
Comment 31•17 years ago
|
||
See also my patch in bug 309424. Perhaps we should combine these two patches.
Comment 32•17 years ago
|
||
(In reply to comment #31) > See also my patch in bug 309424. Perhaps we should combine these two patches. Sounds like a fine idea, Who should review these ?
Updated•17 years ago
|
Attachment #199529 -
Flags: review?(peterv) → review?(jst)
Comment 33•17 years ago
|
||
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop if (!(mState & XML_HTTP_REQUEST_ASYNC)) { while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { + PR_Sleep( PR_MillisecondsToInterval(20)); Seems reasonable, but that should be 10, not 20, right? And loose the space after '('. r=jst
Attachment #199529 -
Flags: superreview?(bzbarsky)
Attachment #199529 -
Flags: review?(jst)
Attachment #199529 -
Flags: review+
Assignee | ||
Comment 34•17 years ago
|
||
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop > // If we're synchronous, spin an event loop here and wait > if (!(mState & XML_HTTP_REQUEST_ASYNC)) { > while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { >+ PR_Sleep( PR_MillisecondsToInterval(20)); > modalEventQueue->ProcessPendingEvents(); > } wouldn't it be better to only sleep if you are going to do another iteration? i.e., how about this: while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { modalEventQueue->ProcessPendingEvents(); // Be sure not to busy wait! (see bug 273578) if (mState & XML_HTTP_REQUEST_SYNCLOOPING) PR_Sleep( PR_MillisecondsToInterval(10)); }
![]() |
||
Comment 35•17 years ago
|
||
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop >Index: nsXMLHttpRequest.cpp > if (!(mState & XML_HTTP_REQUEST_ASYNC)) { > while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { >+ PR_Sleep( PR_MillisecondsToInterval(20)); > modalEventQueue->ProcessPendingEvents(); > } Or even: if (!(mState & XML_HTTP_REQUEST_ASYNC)) { NS_ASSERTION(mState & XML_HTTP_REQUEST_SYNCLOOPING, "Unexpected state"); do { PR_Sleep( PR_MillisecondsToInterval(10)); modalEventQueue->ProcessPendingEvents(); } while (mState & XML_HTTP_REQUEST_SYNCLOOPING); With either that or Darin's suggestion, sr=bzbarsky (note also that 10ms is probably better than 20ms here, unless you had a good reason to pick the 20ms number?).
Attachment #199529 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 36•17 years ago
|
||
I'll check this in with the changes that I suggested.
Assignee | ||
Comment 37•17 years ago
|
||
Here's the version of the patch that I commited on the trunk.
Attachment #199966 -
Flags: approval1.8rc1?
Assignee | ||
Comment 38•17 years ago
|
||
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED
Comment 39•17 years ago
|
||
let's get this verified on the trunk before we consider taking this.
Comment 40•17 years ago
|
||
We've tested this fix with today's nightly build. It works perfect. No more hangs. Thanks for pushing this through, it would be a big help to have this in 1.5, as it's a big problem with our complex AJAX app on linux installs. -Kevin Zimbra
Assignee | ||
Comment 41•17 years ago
|
||
This fix is very low risk. I think we should take it for FF 1.5.
Target Milestone: mozilla1.9alpha → mozilla1.8rc1
Comment 42•17 years ago
|
||
Yeah, this fix is dope. We should not be doing risk management by fix decimation on this particular fix. /be
Comment 43•17 years ago
|
||
Brendan: I'm not sure what you mean by "this fix is dope". Is that a good thing? "Dope" in the UK is marijuana. Is it OK to check it into the branch? If so, can you approve the patch? Gerv
![]() |
||
Comment 44•17 years ago
|
||
"dope" is marijuana in the US too. And it's a slang term for "a good thing", generally... at least when one keeps in mind that Brendan is in California. ;)
Comment 45•17 years ago
|
||
I'm old school. http://dope.urbanup.com/1449519 /be
Updated•17 years ago
|
Attachment #199966 -
Flags: approval1.8rc1? → approval1.8rc1+
You need to log in
before you can comment on or make changes to this bug.
Description
•