8000 extmod/vfs_reader: Increase buffer size for speed boost. by andrewleech · Pull Request #12836 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

extmod/vfs_reader: Increase buffer size for speed boost. #12836

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 9, 2023
Merged

extmod/vfs_reader: Increase buffer size for speed boost. #12836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf7410
extmod/vfs_reader: Add file ioctl to set read buffer size.
pi-anl Oct 31, 2023
bbc5a18
tools/mpremote: Add ioctl to specify large read buffer size.
pi-anl Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions extmod/vfs_reader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#include "py/runtime.h"
#include "py/stream.h"
Expand All @@ -34,33 +35,39 @@

#if MICROPY_READER_VFS

#ifndef MICROPY_READER_VFS_DEFAULT_BUFFER_SIZE
#define MICROPY_READER_VFS_DEFAULT_BUFFER_SIZE (2 * MICROPY_BYTES_PER_GC_BLOCK - offsetof(mp_reader_vfs_t, buf))
#endif
#define MICROPY_READER_VFS_MIN_BUFFER_SIZE (MICROPY_BYTES_PER_GC_BLOCK - offsetof(mp_reader_vfs_t, buf))
#define MICROPY_READER_VFS_MAX_BUFFER_SIZE (255)

typedef struct _mp_reader_vfs_t {
mp_obj_t file;
uint16_t len;
uint16_t pos;
byte buf[24];
uint8_t bufpos;
uint8_t buflen;
uint8_t bufsize;
byte buf[];
} mp_reader_vfs_t;

STATIC mp_uint_t mp_reader_vfs_readbyte(void *data) {
mp_reader_vfs_t *reader = (mp_reader_vfs_t *)data;
if (reader->pos >= reader->len) {
if (reader->len < sizeof(reader->buf)) {
if (reader->bufpos >= reader->buflen) {
if (reader->buflen < reader->bufsize) {
return MP_READER_EOF;
} else {
int errcode;
reader->len = mp_stream_rw(reader->file, reader->buf, sizeof(reader->buf),
&errcode, MP_STREAM_RW_READ | MP_STREAM_RW_ONCE);
reader->buflen = mp_stream_rw(reader->file, reader->buf, reader->bufsize, &errcode, MP_STREAM_RW_READ | MP_STREAM_RW_ONCE);
if (errcode != 0) {
// TODO handle errors properly
return MP_READER_EOF;
}
if (reader->len == 0) {
if (reader->buflen == 0) {
return MP_READER_EOF;
}
reader->pos = 0;
reader->bufpos = 0;
}
}
return reader->buf[reader->pos++];
return reader->buf[reader->bufpos++];
}

STATIC void mp_reader_vfs_close(void *data) {
Expand All @@ -70,18 +77,31 @@ STATIC void mp_reader_vfs_close(void *data) {
}

void mp_reader_new_file(mp_reader_t *reader, qstr filename) {
mp_reader_vfs_t *rf = m_new_obj(mp_reader_vfs_t);
mp_obj_t args[2] = {
MP_OBJ_NEW_QSTR(filename),
MP_OBJ_NEW_QSTR(MP_QSTR_rb),
};
rf->file = mp_vfs_open(MP_ARRAY_SIZE(args), &args[0], (mp_map_t *)&mp_const_empty_map);
int errcode;
rf->len = mp_stream_rw(rf->file, rf->buf, sizeof(rf->buf), &errcode, MP_STREAM_RW_READ | MP_STREAM_RW_ONCE);
mp_obj_t file = mp_vfs_open(MP_ARRAY_SIZE(args), &args[0], (mp_map_t *)&mp_const_empty_map);

const mp_stream_p_t *stream_p = mp_get_stream(file);
int errcode = 0;
mp_uint_t bufsize = stream_p->ioctl(file, MP_STREAM_GET_BUFFER_SIZE, 0, &errcode);
if (bufsize == MP_STREAM_ERROR || bufsize == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For bufsize to be zero, then either:

  • the file's ioctl implementation was invalid (i.e. just returns zero for everything)
  • they wanted the smallest possible buffer

in the first case, it's arbritrary, and in the second case the clamp behavior seems more appropriate, resulting in the min buffer size. So you could skip the bufsize==0 check here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so it turns out the existing mpremote implementation of ioctl is exactly the "invalid" case I described above. So we need this special case. Can you add a comment here explaining this (and probably best fix mpremote as part of this PR too to return -1 for invalid request).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

micropython/tools/mpremote/mpremote/transport_serial.py

Line 747 in e00a144

def ioctl(self, request, arg):

Copy link
Contributor Author
@andrewleech andrewleech Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed that thanks, also added some more unit test coverage
image

// bufsize == 0 is included here to support mpremote v1.21 and older where mount file ioctl
// returned 0 by default.
bufsize = MICROPY_READER_VFS_DEFAULT_BUFFER_SIZE;
} else {
bufsize = MIN(MICROPY_READER_VFS_MAX_BUFFER_SIZE, MAX(MICROPY_READER_VFS_MIN_BUFFER_SIZE, bufsize));
}

mp_reader_vfs_t *rf = m_new_obj_var(mp_reader_vfs_t, buf, byte, bufsize);
rf->file = file;
rf->bufsize = bufsize;
rf->buflen = mp_stream_rw(rf->file, rf->buf, rf->bufsize, &errcode, MP_STREAM_RW_READ | MP_STREAM_RW_ONCE);
if (errcode != 0) {
mp_raise_OSError(errcode);
}
rf->pos = 0;
rf->bufpos = 0;
reader->data = rf;
reader->readbyte = mp_reader_vfs_readbyte;
reader->close = mp_reader_vfs_close;
Expand Down
1 change: 1 addition & 0 deletions py/stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#define MP_STREAM_GET_DATA_OPTS (8) // Get data/message options
#define MP_STREAM_SET_DATA_OPTS (9) // Set data/message options
#define MP_STREAM_GET_FILENO (10) // Get fileno of underlying file
#define MP_STREAM_GET_BUFFER_SIZE (11) // Get preferred buffer size for file

// These poll ioctl values are compatible with Linux
#define MP_STREAM_POLL_RD (0x0001)
Expand Down
18 changes: 17 additions & 1 deletion tests/extmod/vfs_userfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@


class UserFile(io.IOBase):
buffer_size = 16

def __init__(self, mode, data):
assert isinstance(data, bytes)
self.is_text = mode.find("b") == -1
Expand All @@ -39,7 +41,11 @@ def readinto(self, buf):

def ioctl(self, req, arg):
print("ioctl", req, arg)
return 0
if req == 4: # MP_STREAM_CLOSE
return 0
if req == 11: # MP_STREAM_GET_BUFFER_SIZE
return UserFile.buffer_size
return -1


class UserFS:
Expand Down Expand Up @@ -70,6 +76,8 @@ def open(self, path, mode):
"/usermod2.py": b"print('in usermod2')",
"/usermod3.py": b"syntax error",
"/usermod4.mpy": b"syntax error",
"/usermod5.py": b"print('in usermod5')",
"/usermod6.py": b"print('in usermod6')",
}
os.mount(UserFS(user_files), "/userfs")

Expand All @@ -93,6 +101,14 @@ def open(self, path, mode):
except ValueError:
print("ValueError in usermod4")

# Test an import with largest buffer size
UserFile.buffer_size = 255
import usermod5

# Test an import with over-size buffer size (should be safely limited internally)
UserFile.buffer_size = 1024
import usermod6

# unmount and undo path addition
os.umount("/userfs")
sys.path.pop()
16 changes: 16 additions & 0 deletions tests/extmod/vfs_userfs.py.exp
View file A93C
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,37 @@ some data in a text file
stat /usermod1
stat /usermod1.py
open /usermod1.py rb
ioctl 11 0
ioctl 4 0
in usermod1
stat /usermod2
stat /usermod2.py
open /usermod2.py rb
ioctl 11 0
ioctl 4 0
in usermod2
stat /usermod3
stat /usermod3.py
open /usermod3.py rb
ioctl 11 0
ioctl 4 0
SyntaxError in usermod3
stat /usermod4
stat /usermod4.py
stat /usermod4.mpy
open /usermod4.mpy rb
ioctl 11 0
ioctl 4 0
ValueError in usermod4
stat /usermod5
stat /usermod5.py
open /usermod5.py rb
ioctl 11 0
ioctl 4 0
in usermod5
stat /usermod6
stat /usermod6.py
open /usermod6.py rb
ioctl 11 0
ioctl 4 0
in usermod6
7 changes: 7 additions & 0 deletions tools/mpremote/mpremote/transport_serial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
6262 Expand Up @@ -753,6 +753,13 @@ def ioctl(self, request, arg):
machine.mem32[arg] = self.seek(machine.mem32[arg], machine.mem32[arg + 4])
elif request == 4: # CLOSE
self.close()
elif request == 11: # BUFFER_SIZE
# This is used as the vfs_reader buffer. n + 4 should be less than 255 to
# fit in stdin ringbuffer on supported ports. n + 7 should be multiple of 16
# to efficiently use gc blocks in mp_reader_vfs_t.
return 249
else:
return -1
return 0

def flush(self):
Expand Down
0