Performance penalty of using std.io.Reader

[karlseguin]

Zig Version

0.12.0-dev.1396+f6de3ec96

Steps to Reproduce and Observed Behavior

The implementation of std.io.Reader.streamUntilDelimiter is essentially:

const byte: u8 = try self.readByte();
if (byte == delimiter) return;
try writer.writeByte(byte);

Which is a reasonable default implementation, but depending on the underlying context, can leave a lot of performance on the table. Specifically, I noticed that reading a large file, line by line, was really slow, despite using a BufferedReader. Implementing streamUntilDelimiter directly on BufferedReader gave anywhere from a 2x to a 15x performance improvement (depending on the input). Here's a benchmark: https://gist.github.com/karlseguin/17cc66cc26f167c1242608bfae73fb06

I made std.io.GenericReader call an underlying streamUntilDelimiter if present: master...karlseguin:zig:buffered_reader_delimiter

But I couldn't figure out how to push this down to std.io.Reader, where I think it would do the most good (various std.io.Reader functions call streamUntilDelimiter).

So, unable to provide a fix, I thought I'd open an issue.

Expected Behavior

It would be nice if the the performance benefits of using a BufferedReader to remain in play even when hidden behind an io.GenericReader or .io.Reader.

[mlugg]

This may be a performance regression triggered by the creation of the new type-erased reader system. An advantage of the old system was that duplicating all the helper functions (like streamUntilDelimiter) for all readers allowed heavy inlining, which could theoretically tailor these functions to perform better under specific readers.

Someone should look into how this performed prior to the merge of #17344.

[matklad]

Feels like the problem here is that this method exists on Reader at all? In Rust, it is only available for concrete buffered reader, and that seems correct: “reading until a specific byte” isn’t an operation, available on arbitrary reader.

More generally, existence of readByte is highly suspect — this is not an operation that makes sense for something like a TCP stream.

EDIT: Sketched what I think the ideal API should look like here:

• https://gist.github.com/matklad/cc25c31417e73c69f11d174dc2df71c6
• https://ziggit.dev/t/sketch-of-type-safer-buffered-io/4810

[nektro]

I use readByte all the time and when using Reader may not have access to the backing buffered reader. std.io.Reader is a generalization over any reader.

[VoilaNeighbor]

The Reader interface should allow the concrete implementation to provide an optional custom override of helper functions.

[kivikakk]

I use readByte all the time and when using Reader may not have access to the backing buffered reader. std.io.Reader is a generalization over any reader.

It is, but let's say someone does supply a TCP stream, or something else that does no buffering of its own; for any read request less than the packet size, the options are either fail, or return partial data and throw away the remainder (which is a total no-go).

Ergo, remove readByte from Reader — as there's no guarantee any reader can actually do that sensibly — add to BufferedReader, which you'll have to take or instantiate yourself if you want to be able to read from something in a buffered manner.

The alternative and current state is that anyone using Reader can't actually rely on those methods functioning reliably unless they know the implementation can manage it, which imo defeats the purpose of an abstraction.

[notcancername]

@kivikakk

It is, but let's say someone does supply a TCP stream, or something else that does no buffering of its own; for any read request less than the packet size, the options are either fail, or return partial data and throw away the remainder (which is a total no-go).

Huh?

Reads of a single byte are perfectly valid for TCP streams. TCP streams are streams, they are not message-oriented.

shittynetcat.c

#include <unistd.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>

int main(void) {
    const int sock = socket(AF_INET, SOCK_STREAM, 0);

    struct sockaddr_in sin = { .sin_family = AF_INET, .sin_port = htons(1234), .sin_addr = htonl(127 << 24 | 1) };

    connect(sock, (struct sockaddr *)&sin, sizeof (sin));

    while (1) {
        char c;
        const int n = read(sock, &c, 1);
        if (n != 1) break;
        if (write(1, &c, 1) != 1) return 1;
    }
}

printf '%s\n' foo bar baz | nc -l 1234 &

zig run shittynetcat.c -lc

foo
bar
baz