FileIO.readall() reads the file position and size before each call to read(), to adjust the buffer size.

Moreover FileIO.readall() calls lseek() on Windows: it should use _lseeki64() instead, to handle correctly file bigger than 2 GB (or maybe 4 GB? I don't know).

Attached patch fixes both problems.

--

BufferedReader.read() calls FileIO.read() until FileIO.read() returns an empty byte string. Why not calling FileIO.read() only once?

> BufferedReader.read() calls FileIO.read() until FileIO.read() returns
> an empty byte string. Why not calling FileIO.read() only once?

BufferedReader doesn't call FileIO.read, it calls <rawio>.read().
The latter can be e.g. a socket and call recv(). If you want to read
till the end of stream, you have to repeat until recv() returns the
empty string.

> FileIO.readall() reads the file position and size before each call to
> read(), to adjust the buffer size.
> 
> Moreover FileIO.readall() calls lseek() on Windows: it should use
> _lseeki64() instead, to handle correctly file bigger than 2 GB (or
> maybe 4 GB? I don't know).
> 
> Attached patch fixes both problems.

Looks ok to me. Did you test under Windows? Did you run some benchmarks?

Oh, FileIO.readall() doesn't raise a ValueError if the file is closed => patch attached to fix this.

+            raw = f
+            if hasattr(raw, 'buffer'):
+                raw = raw.buffer
+            if hasattr(raw, 'raw'):
+                raw = raw.raw
             f.close()
             self.assertRaises(ValueError, f.flush)
             self.assertRaises(ValueError, f.fileno)
@@ -2512,6 +2517,7 @@ class MiscIOTest(unittest.TestCase):
             self.assertRaises(ValueError, f.read)
             if hasattr(f, "read1"):
                 self.assertRaises(ValueError, f.read1, 1024)
+            self.assertRaises(ValueError, raw.readall)

Why not simply:

             if hasattr(f, "readall"):
                 self.assertRaises(ValueError, f.readall, 1024)

RawIOBase.readall() fails if .read() returns None: fix attached.

bufferedreader_readall.patch: BufferedReader.read(None) calls raw.readall() if available.

bufferedreader_readall.patch requires fileio_readall_closed.patch and rawiobase_readall.patch fixes.

> bufferedreader_readall.patch: BufferedReader.read(None) calls
> raw.readall() if available.

Have you run any benchmarks?

Number of syscalls without the patch -> with the patch:

- read the README file, text mode: 17 -> 12 syscalls (-5)
- read the README file, binary mode: 15 -> 11 syscalls (-4)
- read a binary file of 10 MB: 17 -> 12 syscalls (-5)

Quick benchmark open('README').read(); open('README', 'rb').read() in a loop: 359.1 ms -> 335.4 ms (-7%). So it's not really faster.

About rawiobase_readall.patch: why do you use PyObject_Size() if you know chunks is a list? PyList_GET_SIZE is much more efficient.

About bufferedreader_readall.patch:

+            PyBytes_Concat(&data, all);
+            if (data == NULL)
+                return NULL;

You must Py_DECREF(all) first.

Also, you should check that "all" is either None or a bytes object.

New changeset 742ff94cdd20 by Victor Stinner in branch '3.1':
Issue #12175: FileIO.readall() now raises a ValueError instead of an IOError if
http://hg.python.org/cpython/rev/742ff94cdd20

New changeset 745e373c0b8e by Victor Stinner in branch '3.2':
(Merge 3.1) Issue #12175: FileIO.readall() now raises a ValueError instead of
http://hg.python.org/cpython/rev/745e373c0b8e

New changeset 9051275a68fe by Victor Stinner in branch 'default':
(Merge 3.2) Issue #12175: FileIO.readall() now raises a ValueError instead of
http://hg.python.org/cpython/rev/9051275a68fe

New changeset f2414bb35c96 by Victor Stinner in branch '2.7':
Issue #12175: FileIO.readall() now raises a ValueError instead of an IOError if
http://hg.python.org/cpython/rev/f2414bb35c96

New changeset 4a7118cff1d3 by Victor Stinner in branch '3.1':
Issue #12175: RawIOBase.readall() now returns None if read() returns None.
http://hg.python.org/cpython/rev/4a7118cff1d3

New changeset fb29dc650d24 by Victor Stinner in branch '3.2':
(Merge 3.1) Issue #12175: RawIOBase.readall() now returns None if read()
http://hg.python.org/cpython/rev/fb29dc650d24

New changeset 325453be64ec by Victor Stinner in branch 'default':
(Merge 3.2) Issue #12175: RawIOBase.readall() now returns None if read()
http://hg.python.org/cpython/rev/325453be64ec

New changeset 43a498da8680 by Victor Stinner in branch '2.7':
Issue #12175: RawIOBase.readall() now returns None if read() returns None.
http://hg.python.org/cpython/rev/43a498da8680

> Attached patch [fileio_readall.patch] fixes both problems.

> Looks ok to me. Did you test under Windows?

Yes, test_io pass on Windows Vista 64 bits.

> Did you run some benchmarks?

Yes, see msg136853. Do you mean that I should not touch FileIO.readall() if it doesn't make it faster? (only 7% faster)

> You must Py_DECREF(all) first.
> Also, you should check that "all" is either None or a bytes object.

Right, fixed in bufferedreader_readall-2.patch. By the way, thanks for your reviews.

So, do you think that fileio_readall.patch and bufferedreader_readall-2.patch should be commited, or these changes are useless?

> So, do you think that fileio_readall.patch and
> bufferedreader_readall-2.patch should be commited, or these changes
> are useless?

They should be committed.
Thank you :)

New changeset 72c89daace57 by Victor Stinner in branch 'default':
Issue #12175: FileIO.readall() now only reads the file position and size once.
http://hg.python.org/cpython/rev/72c89daace57

New changeset 3c7792ec4547 by Victor Stinner in branch 'default':
Issue #12175: BufferedReader.read(-1) now calls raw.readall() if available.
http://hg.python.org/cpython/rev/3c7792ec4547