FileIO.readall() read the file position and size at each read

[vstinner]

BPO	12175
Nosy	@pitrou, @vstinner
Files	fileio_readall.patch bufferedreader_readall-2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2011-05-30.00:30:52.409>
created_at = <Date 2011-05-25.12:13:59.954>
labels = ['expert-IO']
title = 'FileIO.readall() read the file position and size at each read'
updated_at = <Date 2011-05-30.00:30:52.408>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2011-05-30.00:30:52.408>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2011-05-30.00:30:52.409>
closer = 'vstinner'
components = ['IO']
creation = <Date 2011-05-25.12:13:59.954>
creator = 'vstinner'
dependencies = []
files = ['22105', '22118']
hgrepos = []
issue_num = 12175
keywords = ['patch']
message_count = 17.0
messages = ['136840', '136842', '136843', '136845', '136846', '136848', '136850', '136851', '136853', '136855', '136891', '136892', '136897', '136902', '136903', '136909', '136912']
nosy_count = 3.0
nosy_names = ['pitrou', 'vstinner', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue12175'
versions = ['Python 3.3']

[vstinner]

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?

[pitrou]

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.

[pitrou]

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?

[vstinner]

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

[pitrou]

+            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)

[vstinner]

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

[vstinner]

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

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

[pitrou]

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

Have you run any benchmarks?

[vstinner]

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.

[pitrou]

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.

[python-dev]

New changeset 742ff94cdd20 by Victor Stinner in branch '3.1':
Issue bpo-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 bpo-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 bpo-12175: FileIO.readall() now raises a ValueError instead of
http://hg.python.org/cpython/rev/9051275a68fe

[python-dev]

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

[python-dev]

New changeset 4a7118cff1d3 by Victor Stinner in branch '3.1':
Issue bpo-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 bpo-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 bpo-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 bpo-12175: RawIOBase.readall() now returns None if read() returns None.
http://hg.python.org/cpython/rev/43a498da8680

[vstinner]

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)