408722 - Reading files with File object (jsfile.c) is incredibly slow

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Wesley W. Garland]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Spidermonkey, cvs-latest

On my platform it, it takes over 10 seconds to read a "A Tale of Two Cities" from Project Gutenberg (800KB of text) with File.prototype.readAll().

My platform is a Solaris 10 zone running on a lightly-loaded Sun Enterprise 420R server configured with 2 450MHz CPUs, doing disk over NFS. The OS can copy that same file to RAM (/tmp) in 0.02 real / 0.01 sys seconds.

Reproducible: Always

Steps to Reproduce:
Write a JavaScript program of this nature:

var file = new File("A Tale of Two Cities.txt");
file.open("read", "text");
file.readAll();
file.close();

and run it.
Actual Results:  
It takes about 10 seconds to run.

Expected Results:  
It should take far less than that. Perhaps one or two seconds would be a reasonable goal?

truss reveals that the JavaScript engine is reading the file a single byte at a time.

Note: This component is not part of the build, and must be explicitly enabled by an embedder when Spidermonkey is compiled.

Comment 1

[Wesley W. Garland]

Attachment: A patch to add buffered input to the File object

This patch implements buffered input, thereby reducing the number of times the read() system call is used.

This has a significant (positive) performance impact, and reads the same file on the same system (with a debug build of spidermonkey) in ~3s.

It is implemented as an NSPR IO layer, and is only enabled if the JavaScript programmer requests it at run time, by selecting the "buffered" option in File.prototype.open().

Note -- as there appears to be no test suite for this object, I have created a quick-and-dirty verifier to measure my changes and reduce the probability that I have introduced errors.  It is online at http://www.page.ca/~wes/opensource/jsfile_testsuite

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 293569 [details] [diff] [review]
A patch to add buffered input to the File object

>+static PRInt32
>+jsfile_buffered_PR_Read(PRFileDesc *fd, void *rbuf, PRInt32 amount)
>+            if (buffer->finalReadResult)
>+                return buffer->finalReadResult;
>+
>+            res = fd->lower->methods->read(fd->lower, buffer->data, sizeof(buffer->data));
>+            if (res <= 0) {
>+                buffer->finalReadResult = res;

FWIW, we could end up here several times if finalReadResult is EOF (i.e. 0). Does that matter?

>+        }
>+
>+        *rbufp++ = *buffer->next++;
>+

Nit: extra newline here.

>+static PRInt32
>+jsfile_buffered_PR_Seek(PRFileDesc *fd, PRInt32 offset, PRSeekWhence whence)
>+{
> ...
>+    switch(whence) {
> ...
>+      case PR_SEEK_END:
>+        goto hard_seek_and_invalidate_buffer;

So, here newOffset is uninitialized...

>+    }
>+
>+    /* no move? */

Nit: I know this file is inconsistent, but, in general, comments in SpiderMonkey are complete sentences, starting with a capital. Might be good to move in that direction. This comment applies to a couple of other comments you've added.

>+    if ((buffer->next >= buffer->data) && (buffer->next < buffer->end))
>+        return newOffset;

Nit: the condition is overparenthesized.

>+hard_seek_and_invalidate_buffer:
>+    /* hard seek and invalidate buffer */
>+    res = fd->lower->methods->seek(fd->lower, newOffset, PR_SEEK_SET);

...In the case where whence is PR_SEEK_END, we're now passing the uninitialized newOffset with PR_SEEK_SET. I don't think that's what you want.

>+static PRInt32
>+jsfile_buffered_PR_Write(PRFileDesc *fd, const void *buf, PRInt32 amount)
>+{
> ...
>+
>+

Nit: extra newline.

>+            rv = PR_PushIOLayer(file->handle, PR_TOP_IO_LAYER, layer);
>+            JS_ASSERT(PR_SUCCESS == rv);

This *really* can't fail? Seems like it might be worth handling this more gracefully.

Looks pretty good otherwise. I'll stamp the next patch.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 293569 [details] [diff] [review]
A patch to add buffered input to the File object

Marking r- for the uninitialized variable problem and to get this out of my queue.

Comment 4

[Wesley W. Garland]

tracemonkey has deprecated jsfile.cpp in its entirety: marking WONTFIX