homepage

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