Two possible improvements in std.fs.File.readToEndAllocOptions

[nunokaeru]

Zig Version

0.14.0-dev.1573+4d81e8ee9

Steps to Reproduce and Observed Behavior

The issues refers to the following function:

/// Reads all the bytes from the current position to the end of the file.
/// On success, caller owns returned buffer.
/// If the file is larger than `max_bytes`, returns `error.FileTooBig`.
/// If `size_hint` is specified the initial buffer size is calculated using
/// that value, otherwise an arbitrary value is used instead.
/// Allows specifying alignment and a sentinel value.
pub fn readToEndAllocOptions(
    self: File,
    allocator: Allocator,
    max_bytes: usize,
    size_hint: ?usize,
    comptime alignment: u29,
    comptime optional_sentinel: ?u8,
) !(if (optional_sentinel) |s| [:s]align(alignment) u8 else []align(alignment) u8) {
    // If no size hint is provided fall back to the size=0 code path
    const size = size_hint orelse 0;

    // The file size returned by stat is used as hint to set the buffer
    // size. If the reported size is zero, as it happens on Linux for files
    // in /proc, a small buffer is allocated instead.
    const initial_cap = (if (size > 0) size else 1024) + @intFromBool(optional_sentinel != null);
    var array_list = try std.ArrayListAligned(u8, alignment).initCapacity(allocator, initial_cap);
    defer array_list.deinit();

    self.reader().readAllArrayListAligned(alignment, &array_list, max_bytes) catch |err| switch (err) {
        error.StreamTooLong => return error.FileTooBig,
        else => |e| return e,
    };

    if (optional_sentinel) |sentinel| {
        return try array_list.toOwnedSliceSentinel(sentinel);
    } else {
        return try array_list.toOwnedSlice();
    }
}

Issue 1:

• Sentinel value does not need to be added to initial cap because toOwnedSliceSentinel will create the +1 memory for it. In line: const initial_cap = (if (size > 0) size else 1024) + @intFromBool(optional_sentinel != null);

Issue 2:

• The following comment might not be correct as we do not use ( try f.stat() ).size as an hint to the initial_cap.

    // The file size returned by stat is used as hint to set the buffer
    // size. If the reported size is zero, as it happens on Linux for files
    // in /proc, a small buffer is allocated instead.
    const initial_cap = (if (size > 0) size else 1024) + @intFromBool(optional_sentinel != null);
    var array_list = try std.ArrayListAligned(u8, alignment).initCapacity(allocator, initial_cap);

Interestingly enough fmtPathFile does exactly what the comment refers to.

fn fmtPathFile(
    fmt: *Fmt,
    file_path: []const u8,
    check_mode: bool,
    dir: fs.Dir,
    sub_path: []const u8,
) FmtError!void {
    const source_file = try dir.openFile(sub_path, .{});
    var file_closed = false;
    errdefer if (!file_closed) source_file.close();

    const stat = try source_file.stat();

    if (stat.kind == .directory)
        return error.IsDir;

    const gpa = fmt.gpa;
    const source_code = try std.zig.readSourceFileToEndAlloc(
        gpa,
        source_file,
        std.math.cast(usize, stat.size) orelse return error.FileTooBig,
    );
    defer gpa.free(source_code);

Expected Behavior

The inital size of the array list to match exactly the size of the file when allowed.

[nektro]

are there any situations where a filesystem might throw an error on trying to stat but would succeed on read?

[nunokaeru]

are there any situations where a filesystem might throw an error on trying to stat but would succeed on read?

I do not know. A possible implementation to take that into account is to catch the error from the stat() call and if it happens use the default small buffer as we do currently.

[ianprime0509]

Regarding issue 1, the purpose of the +1 in the initial capacity is to avoid an additional allocation when appending the sentinel value. In the ideal case with the current code, if the actual size of the data read from the file matches the size hint, then only the initCapacity call needs to allocate. Then, when it is time to append the sentinel in toOwnedSliceSentinel:

zig/lib/std/array_list.zig

Line 130 in 3b465eb

try self.ensureTotalCapacityPrecise(self.items.len + 1);

this call to ensureTotalCapacityPrecise will not need to do any allocation, since it will check the requested capacity (length + 1) against the current capacity (in the ideal case, the size hint + 1), and will return early if the request is already satisfied:

zig/lib/std/array_list.zig

Line 464 in 3b465eb

if (self.capacity >= new_capacity) return;

[nunokaeru]

Thanks, I strikethroughed the issue on the original post and fixed the Pull request with the suggestion

[squeek502]

Interestingly enough fmtPathFile does exactly what the comment refers to

I believe that's the intention. In general, Zig tries to minimize the number of syscalls made, so leaving the stat call up to the caller of readToEndAllocOptions is in line with that goal.

[nunokaeru]

Interestingly enough fmtPathFile does exactly what the comment refers to

I believe that's the intention. In general, Zig tries to minimize the number of syscalls made, so leaving the stat call up to the caller of readToEndAllocOptions is in line with that goal.

I agree. However the comment in readToEndAllocOptions is not correct because we do not take the size of the file into account when allocating memory even though the comment mentions that.

    // The file size returned by stat is used as hint to set the buffer
    // size. If the reported size is zero, as it happens on Linux for files
    // in /proc, a small buffer is allocated instead.
    const initial_cap = (if (size > 0) size else 1024) + @intFromBool(optional_sentinel != null);

I felt this was strange to be here because I didn't see any call to stat() currently, I assume the intention of the comment is correct but somewhere along the way the call to stat() and the initialization of the array with enough memory for all the elements of the file was lost.

Note: without the removal of size_hint the caller is still in full control of the initial allocated memory but by using stat().size the default option will be better now.

PS: Updated the pull request with your recommendation.

[squeek502]

the comment in readToEndAllocOptions is not correct

Seems like updating the comment might be a viable alternative to adding a stat call. I'm imagining something like:

    // It is assumed that `size_hint` will usually come from `stat`,
    // so if the reported size is zero, as it happens on Linux for files
    // in /proc, a small buffer is allocated instead.

EDIT: For context, there was a stat call in readToEndAllocOptions at one point, but it was removed in 9074388 when size_hint was added

[nunokaeru]

@LemonBoy any thoughts?

I can settle for just updating the comment but IMO it's a good default to have stat().size as the initial capacity when size_hint is not provided.

[andrewrk]

LemonBoy hasn't been active here for a long time. I don't think he would appreciate that ping out of nowhere. Better to just discuss issues and let people decide to subscribe to notifications if they want to.