Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open a file in text mode requires too many syscalls #74414

Closed
vstinner opened this issue May 2, 2017 · 6 comments
Closed

Open a file in text mode requires too many syscalls #74414

vstinner opened this issue May 2, 2017 · 6 comments
Labels
3.7 (EOL) end of life performance Performance or resource usage topic-IO

Comments

@vstinner
Copy link
Member

vstinner commented May 2, 2017

BPO 30228
Nosy @pitrou, @vstinner, @serhiy-storchaka
PRs
  • bpo-30228: FileIO seek() and tell() set seekable #1384
  • bpo-30228: TextIOWrapper uses abs_pos, not tell() #1385
  • 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 2017-09-16.01:28:23.843>
    created_at = <Date 2017-05-02.11:11:25.828>
    labels = ['3.7', 'expert-IO', 'performance']
    title = 'Open a file in text mode requires too many syscalls'
    updated_at = <Date 2017-09-16.01:29:24.451>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-09-16.01:29:24.451>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-09-16.01:28:23.843>
    closer = 'vstinner'
    components = ['IO']
    creation = <Date 2017-05-02.11:11:25.828>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30228
    keywords = []
    message_count = 6.0
    messages = ['292744', '292752', '292759', '292881', '292882', '302307']
    nosy_count = 3.0
    nosy_names = ['pitrou', 'vstinner', 'serhiy.storchaka']
    pr_nums = ['1384', '1385']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue30228'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    vstinner commented May 2, 2017

    Example:

    with open("x", "w", encoding="utf-8") as fp:
        fp.write("HERE")
        fp.close()

    syscalls:

    14249 open("x", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 3
    14249 fstat(3, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
    14249 ioctl(3, TCGETS, 0x7fff07d43330) = -1 ENOTTY (Inappropriate ioctl for device)
    14249 lseek(3, 0, SEEK_CUR) = 0
    14249 lseek(3, 0, SEEK_CUR) = 0
    14249 lseek(3, 0, SEEK_CUR) = 0
    14249 write(3, "HERE", 4) = 4
    14249 close(3) = 0

    I only expected 3 syscalls: open, write, close.

    • fstat() is used by the FileIO constructor to check if the file is a directory or not, and to get the block size
    • ioctl() comes from open() which checks if the file is a TTY or not, to decide how to configure buffering
    • the first lseek() is used by the BuffererWriter constructor to initialize the private abs_pos attribute
    • the second lseek() is used by the TextIOWrapper constructor to check if the underlying file object (buffered writer) is seekable or not
    • the last lseek() is used to create the cookie object in TextIOWrapper constructor

    Can we maybe reduce the number of lseek() to a single syscall?

    For example, BuffererWriter constructor calls FileIO.tell(): can't this method set the seekable attribute depending on lseek() success, as the FileIO.seekable property?

    @vstinner vstinner added 3.7 (EOL) end of life performance Performance or resource usage topic-IO labels May 2, 2017
    @vstinner
    Copy link
    Member Author

    vstinner commented May 2, 2017

    New changeset 9997073 by Victor Stinner in branch 'master':
    bpo-30228: FileIO seek() and tell() set seekable (bpo-1384)
    9997073

    @serhiy-storchaka
    Copy link
    Member

    I don't like PR 1385. abs_pos is a private attribute used only in _io._Buffered.seek() for readable streams when whence is SEEK_SET or SEEK_CUR. There is no guarantee that it contains relevant value for non-readable stream.

    You could call buffer.seek(0, SEEK_CUR) rather than buffer.tell() for avoiding a system call for readable stream. But this looks as a shamanism too.

    Or provide a function similar to the RAW_TELL macro but just checking if the current position is 0. If define it in bufferedio.c near _buffered_raw_tell() it is more chance that it is consistent with abs_pos and future changes don't break it.

    @vstinner
    Copy link
    Member Author

    vstinner commented May 3, 2017

    You could call buffer.seek(0, SEEK_CUR) rather than buffer.tell() for avoiding a system call for readable stream. But this looks as a shamanism too.

    Note: Buffered.seek(0, SEEK_CUR) only has a fast-path for readable file: it cannot be used to optimize open(filename, "w") (BufferedWriter.seek() isn't optimized).

    @vstinner
    Copy link
    Member Author

    vstinner commented May 3, 2017

    Or provide a function similar to the RAW_TELL macro but just checking if the current position is 0.

    I will try to implement such function and use it in textio.c.

    @vstinner
    Copy link
    Member Author

    Microbenchmark on Fedora 26 for #1385

    Working directly uses ext4, the filesystem operations are likely cached in memory, so syscalls should be very fast.

    $ ./python -m perf timeit --inherit=PYTHONPATH 'open("x.txt", "w").close()' -o open_ref.json -v
    $ ./python -m perf timeit --inherit=PYTHONPATH 'open("x.txt", "w").close()' -o open_patch.json -v
    $ ./python -m perf compare_to open_ref.json open_patch.json 
    Mean +- std dev: [open_ref] 18.6 us +- 0.2 us -> [open_patch] 18.2 us +- 0.2 us: 1.02x faster (-2%)

    Microbenchmark using a btrfs filesystem mounted on NFS over wifi: not significant!

    $ ./python -m perf timeit --inherit=PYTHONPATH 'open("nfs/x.txt", "w").close()' --append open_patch.json -v
    $ ./python -m perf timeit --inherit=PYTHONPATH 'open("nfs/x.txt", "w").close()' --append open_patch.json -v
    haypo@selma$ ./python -m perf compare_to open_ref.json open_patch.json  -v
    Mean +- std dev: [open_ref] 17.8 ms +- 1.0 ms -> [open_patch] 17.8 ms +- 1.0 ms: 1.00x faster (-0%)
    Not significant!

    Note: open().close() is 1000x slower over NFS!

    According to strace, on NFS, open() and close() are slow, but syscalls in the middle are as fast as syscalls on a local filesystem.

    Well, it's hard to see a significant speedup, even on NFS. So I abandon my change.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life performance Performance or resource usage topic-IO
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants