Closed Bug 584610 Opened 14 years ago Closed 14 years ago

RecvLoadRemoteScript uses a tiny buffer to load script

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(2 files, 1 obsolete file)

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
got numbers to justify a change?  will moving to use the omnijar make a difference here?
nsInProcessTabChildGlobal has the same thing.
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.
Alon, thanks for the report. What OS were you running the tests on?
(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.
i'd like to see numbers on a device before closing this out.
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.
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
post a patch, brother
Attached patch patchSplinter Review
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 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 on attachment 469579 [details] [diff] [review]
patch

Actually, I'm not yet sure whether this works always.
Attachment #469579 - Flags: review+ → review?
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.
(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?
(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.
The question is that does the inputstream always know the entire size of the
whatever is being loaded.
(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
Attached patch alternative patch (obsolete) — Splinter Review
Attachment #469649 - Flags: review?(Olli.Pettay)
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
Attachment #469579 - Flags: review?
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+
Tested with some smaller numbers. 8192 seems to work well. Checking in with bufferSize = 8192, instead of 32678.
tracking-fennec: --- → 2.0+
We do have:  network.buffer.cache.size

which is marginally better than what you are doing.
(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.
carrying r+ forward
Attachment #469649 - Attachment is obsolete: true
Attachment #469928 - Flags: review+
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.

Attachment

General

Created:
Updated:
Size: