Prevent extraneous fstat during open()

[bkabrda]

BPO	21679
Nosy	@pitrou, @benjaminp, @vadmium, @serhiy-storchaka, @MojoVampire
Files	python3-remove-extraneous-fstat-on-file-open.patch python3-remove-extraneous-fstat-on-file-open-v2.patch python3-remove-extraneous-fstat-on-file-open-v3.patch python3-remove-extraneous-fstat-on-file-open-v4.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 2014-06-30.00:13:29.653>
created_at = <Date 2014-06-06.11:48:05.445>
labels = ['expert-IO', 'performance']
title = 'Prevent extraneous fstat during open()'
updated_at = <Date 2015-11-24.08:37:58.383>
user = 'https://bugs.python.org/bkabrda'

bugs.python.org fields:

activity = <Date 2015-11-24.08:37:58.383>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2014-06-30.00:13:29.653>
closer = 'pitrou'
components = ['IO']
creation = <Date 2014-06-06.11:48:05.445>
creator = 'bkabrda'
dependencies = []
files = ['35494', '35540', '35548', '35702']
hgrepos = []
issue_num = 21679
keywords = ['patch']
message_count = 10.0
messages = ['219874', '220084', '220152', '220706', '220752', '220810', '221071', '221923', '221925', '255253']
nosy_count = 8.0
nosy_names = ['pitrou', 'benjamin.peterson', 'stutzbach', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'bkabrda', 'josh.r']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue21679'
versions = ['Python 3.5']

[bkabrda]

Hi,
with Python 3.3/3.4, I noticed that there are lots of syscalls on open() - I noticed 2x fstat, 2x ioctl and 2x lseek. This is not noticable when working with small amounts of files on local filesystem, but if working with files via NSF or if working with huge amounts of files, lots of syscalls cost a lot of time. Therefore I'd like to create patches that would reduce the number of syscalls on open().
I've already managed to create first one (attached), that gets rid of one of the fstat calls (all the information are obtained from one fstat call).
I hope this makes sense and that the patch is acceptable. If not, I'll be happy to work on it to make it better. (This is my first real patch for C part of Python, so I hope I did everything right...)

[bkabrda]

Thanks a lot for the code review! I'm attaching a revised version of the patch. Fixes I made:

• added check whether PyLong_AsLong returned an error
• removed "ADD_INTERNED(_blksize)" and "PyObject *_PyIO_str__blksize;" - I noticed that these are only necessary when exported by _iomodule.h, which isn't needed for _blksize ATM
• moved blksize to a place of fileio structure where it won't create unnecessary padding

I hope attaching the version 2 of the patch here is ok, if I should have attached it in the code review tool somehow, please let me know.

[bkabrda]

Again, thanks for the review. It's true that HAVE_FSTAT can be defined without stat structure containing st_blksize. I added an ifdef HAVE_STRUCT_STAT_ST_BLKSIZE for that. Attaching third version of the patch, hopefully everything will be ok now.

[bkabrda]

So, as pointed out by haypo, blksize_t is actually signed long on Linux. This means that my patch (as well as the current code) is not right.
Both with and without my patch, io_open function uses "int" to store blksize_t and it also passes it to one of PyBuffered{Random,Reader,Writer}_Type. These three however use Py_ssize_t, which is inconsistent with io_open, and it's not correct, too.
I'm unsure how to proceed here - should I fix buffer size types throughout the _io module to long and submit one big patch? It doesn't feel right to put two not-very-related changes into one patch (I'm afraid that changing buffer sizes to long everywhere will result in a rather large patch). Or should I first submit a patch that fixes the long usage and then rebase the patch attached to this issue on top of it?

Thanks a lot.

[pitrou]

I think it's ok to keep the block size an int for now. It would be surprising for a block size to be more than 2 GB, IMO.

[bkabrda]

Thanks, Antoine. So, is there anything else that should be done about the patch so that it gets accepted?

[bkabrda]

I'm attaching fourth version of the patch. Changes:

• fileio's _blksize member is now of type T_UINT
• extended test_fileio.AutoFileTests.testAttributes to also test _blksize value

[python-dev]

New changeset 3b5279b5bfd1 by Antoine Pitrou in branch 'default':
Issue bpo-21679: Prevent extraneous fstat() calls during open(). Patch by Bohuslav Kabrda.
http://hg.python.org/cpython/rev/3b5279b5bfd1

[pitrou]

Thank you very much. I've committed the patch to the default branch (I've just moved the _blksize test to a separate method).

[vadmium]

The change made here no longer tolerates fstat() returning an error, which seems to be the cause of bpo-25717.