Closed
Bug 584610
Opened 14 years ago
Closed 14 years ago
RecvLoadRemoteScript uses a tiny buffer to load script
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0+ | --- |
People
(Reporter: mfinkle, Assigned: mfinkle)
Details
Attachments
(2 files, 1 obsolete file)
1.41 KB,
patch
|
Details | Diff | Splinter Review | |
837 bytes,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
TabChild::RecvLoadRemoteScript uses a 256 byte buffer to read the frame script. We can surely uses a larger buffer, or just read the entire file at once. We have seen the negative affect of small read buffers in other bugs, especially in mobile. See: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#907
Comment 1•14 years ago
|
||
got numbers to justify a change? will moving to use the omnijar make a difference here?
Comment 2•14 years ago
|
||
nsInProcessTabChildGlobal has the same thing.
Comment 3•14 years ago
|
||
I set up a profiling environment for other purposes, which turned out useful for this. Changing the buffer from 256 to 4096 moves the time from 0.0717ms to 0.0631ms. So, 12% improvement (on something rather small, though). Tested on the loadFrameScript calls made in bug 550936.
Assignee | ||
Comment 4•14 years ago
|
||
Alon, thanks for the report. What OS were you running the tests on?
Comment 5•14 years ago
|
||
(In reply to comment #4) > Alon, thanks for the report. What OS were you running the tests on? Ubuntu 10.04 on a MacBook Pro.
Comment 6•14 years ago
|
||
i'd like to see numbers on a device before closing this out.
Assignee | ||
Comment 7•14 years ago
|
||
Yes, numbers on a device are important. Taras did some of our early file IO profiling on devices and saw very poor performance. I think he even looked into mmaping files or reading the entire file in a single read.
Assignee | ||
Comment 8•14 years ago
|
||
The code changed and the buffer size is now 1024. I tested changing the buffer size to 32768 and ran some tests on device: 1024 buffer Ts: 8186 ms (avg of 6 runs) 32768 buffer Ts: 7831 ms (avg of 6 runs) 355 ms improvement on my device
Comment 9•14 years ago
|
||
post a patch, brother
Assignee | ||
Comment 10•14 years ago
|
||
This patch uses the same approach as the Components.utils.import code, found here: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1430 It gets the size of the stream and loads the contents in one swoop. Ts after 6 runs = 7855 ms
Assignee: nobody → mark.finkle
Attachment #469579 -
Flags: review?(Olli.Pettay)
Comment 11•14 years ago
|
||
Comment on attachment 469579 [details] [diff] [review] patch >+ /* read the file in one swoop */ >+ rv = input->Read(buffer, len, &bytesRead); >+ if (len != bytesRead) Could you add some warning here.
Attachment #469579 -
Flags: review?(Olli.Pettay) → review+
Comment 12•14 years ago
|
||
Comment on attachment 469579 [details] [diff] [review] patch Actually, I'm not yet sure whether this works always.
Attachment #469579 -
Flags: review+ → review?
Comment 13•14 years ago
|
||
drive-by comment >+ PRUint32 len, bytesRead; >+ input->Available(&len); >+ if (len) { >+ /* malloc an internal buf the size of the file */ >+ nsAutoArrayPtr<char> buffer(new char[len + 1]); >+ if (!buffer) >+ return; NS_ABORT_IF_FALSE(0, "Could not allocate buffer for frame script"); mostly for debugging purposes. >+ >+ /* read the file in one swoop */ >+ rv = input->Read(buffer, len, &bytesRead); No one cares about rv. Maybe delete it from this method completely. >+ buffer[len] = '\0'; >+ data.Append(buffer, bytesRead); Do we really have to copy to data here? Also this means that the input stream being passed here must have all of the data ready to go. this might be fine for now, but it does mean that read-until-EOF input streams will not work.
Comment 14•14 years ago
|
||
(In reply to comment #12) > Comment on attachment 469579 [details] [diff] [review] > patch > > Actually, I'm not yet sure whether this works always. Or in other words, could you explain why the patch works?
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #14) > (In reply to comment #12) > > Comment on attachment 469579 [details] [diff] [review] [details] > > patch > > > > Actually, I'm not yet sure whether this works always. > > Or in other words, could you explain why the patch works? Hmm, I never questioned that it wouldn't work :) I figured that if increasing the buffer to 32K, as I did earlier, worked - why not try to read the entire buffer at once. I looked and saw that Components.utils.import was doing the same thing, or what appeared to be the same thing, so I copied the approach. It works fine for Fennec. Why wouldn't this work but a fixed buffer size would? I can change to a fixed buffer size if you like.
Comment 16•14 years ago
|
||
The question is that does the inputstream always know the entire size of the whatever is being loaded.
Assignee | ||
Comment 17•14 years ago
|
||
(In reply to comment #16) > The question is that does the inputstream always know the entire size of the > whatever is being loaded. Ah. So previously, the code asked for input->Available(&avail), which would return a number, but not necessarily the real size. I'll post a patch using the simple bufferSize bump
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #469649 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 19•14 years ago
|
||
Comment on attachment 469649 [details] [diff] [review] alternative patch This patch is a simple bufferSize increase. We still get the same level of Ts improvement
Assignee | ||
Updated•14 years ago
|
Attachment #469579 -
Flags: review?
Comment 20•14 years ago
|
||
Comment on attachment 469649 [details] [diff] [review] alternative patch Did you try anything smaller like 4096? or 8192?
Attachment #469649 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 21•14 years ago
|
||
Tested with some smaller numbers. 8192 seems to work well. Checking in with bufferSize = 8192, instead of 32678.
Updated•14 years ago
|
tracking-fennec: --- → 2.0+
Comment 22•14 years ago
|
||
We do have: network.buffer.cache.size which is marginally better than what you are doing.
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #22) > We do have: network.buffer.cache.size > > which is marginally better than what you are doing. Debatable, but adding a specific pref is a great followup bug.
Assignee | ||
Comment 24•14 years ago
|
||
carrying r+ forward
Attachment #469649 -
Attachment is obsolete: true
Attachment #469928 -
Flags: review+
Comment 25•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/256514ea6d14
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•