The type erased reader changes massively impact zig's IO performance for buffered readers, fixed buffer streams, etc.

[tomc1998]

I'm using 0.13.0 stable, although from browsing the source on master the issue is still present.

The type erased reader changes (#17344) need to be reverted: there is currently no way to efficiently do IO with zig's stdlib, IO API feels like a performance trap.

Prior to the type-erased reader changes, if I need to write a function that parses a file, I take a reader by anytype and implement the function on that. At compile time I could switch out a BufferedReader or FixedBufferStream reader and still get good performance.

Now however, the performance is significantly worse after 0.13.0. I've got a function which loads java .class files and parses them. I'm seeing a 2x performance loss comparing the current type-erased FixedBufferStream and a custom Reader implementation without any dynamic calls. This is with many complex allocations to store the results of the parse: the reader performance issue is worse if I'm doing something simpler with fewer allocations. You can also see these results in other issues that have been opened: #21566, #17985.

This is because the type erased reader changes prevents anything from being inlined: so when profiling an IO function I can see a lot of samples in readByte, for example. I would expect any calls to readByte on a FixedBufferStream to be inlined and optimized to a couple instructions, so any reader functions being present in the profile is a bad sign. AFAIK prior to 0.13.0 this inlining happened; I never saw these functions in the profile, and performance was much better.

In 0.13.0, any time I use the zig stdlib IO reader/writer APIs I regret it, which is unfortunate.

[andrewrk]

It's not going to be reverted since there is value in having a non generic reader and writer interface.

Performance issues will need to be solved a different way, such as:

• bifurcating the API into a generic and non generic one, making the generic no longer wrap the non-generic one
• better devirtualization optimization
• eliminating readByte from everything except the buffered reader

I'll leave the issue open for tracking the problem.

[tomc1998]

bifurcating the API into a generic and non generic one, making the generic no longer wrap the non-generic one

I'm getting confused - you say 'generic reader' to mean 'comptime templated reader', right? The one with no dynamic dispatch?

If so: this would be my preferred solution. I use Zig as a replacement for C, and do not want to rely on optimizations to get reasonable performance.

In the old system, you gave Reader a read() function at comptime, and that generated a new type. If you want a dynamic-dispatched reader that worked against a dynamic fnptr, then why not create an AnyReader which just calls through to your dynamic read() fnptr and leave the old Reader API as it was?

/// A dynamic-dispatch reader which reads through a function pointer
const AnyReader = struct {
    const StaticReader = std.io.Reader(@This(), anyerror, wrapperReadFn);
    context: *const anyopaque,
    readFn: *const fn (context: *const anyopaque, buffer: []u8) anyerror!usize,
    fn wrapperReadFn(ctx: @This(), buffer: []u8) !usize {
        return ctx.readFn(ctx.context, buffer);
    }
    pub fn reader(self: @This()) StaticReader {
        return StaticReader { .context = self };
    }
};

Is there a reason why it wasn't done this way? If you want a dynamic dispatch reader for any reason, you could easily create one - if you want a static one, you don't pay any perf cost

[andrewrk]

I'm getting confused - you say 'generic reader' to mean 'comptime templated reader', right? The one with no dynamic dispatch?

Yes that's what I mean, sorry for the ambiguity. The one that requires functions to be generic.

do not want to rely on optimizations to get reasonable performance.

I would expect any calls to readByte on a FixedBufferStream to be inlined and optimized to a couple instructions

These are contradictory statements.

The only difference between the type erased version and templated version is friendliness to optimization, presumably due to the lack of runtime-known function pointers in the latter case.

You're measuring the perf difference in optimized builds, not debug builds. In debug builds the type-erased one will perform strictly better, since currently the templated one wraps the type-erased one.

You are, in fact, wanting to rely on optimizations to get reasonable performance. That's the entire point of why you opened this issue.

[tomc1998]

These are contradictory statements.

Technically they are, but I think we both know what I meant. Relying on a compiler inlining code within a single translation unit is far less brittle than something like devirtualization, and the performance cliff is much smaller.

With devirtualization, the further the reader creation site gets from its usage, the more likely it is for the devirtualization optimization to fail and be left with a dynamic dispatch - which then cascades and prevents inlining, etc. You can suddenly get huge performance cliffs on unrelated changes. Having to rely on this kind of optimization concerns me.

In debug builds the type-erased one will perform strictly better, since currently the templated one wraps the type-erased one.

I would expect the type-erased one to perform worse than the old, non-type erased one in debug builds that contains only static calls - although probably by less of a margin.

---

I would be interested in doing this work, it's currently a bugbear for me. I think the AnyReader setup I proposed above is reasonable (although I'm happy to hear otherwise). What would the path be for me to take this proposal further, make a proposal issue & argue over there?

I notice you tagged this issue as 0.16.0. Does that mean you won't be accepting proposals until there's some more churn here, and I should come back later?

[andrewrk]

I notice you tagged this issue as 0.16.0. Does that mean you won't be accepting proposals until there's some more churn here, and I should come back later?

I use the milestones to track estimates of when I think the zig core dev team will get an issue solved. So it's more like, "if a PR doesn't get merged that solves this by then, then I'll look at it myself."

You're welcome to submit something, and I will be happy to have a look at it, however, I can't promise such efforts will not be wasted - there is a chance it takes me a long time to get around to it, or that I decide to go a different direction with the API or implementation.

[gwenzek]

The approach I'm using is to favor Buffered(AnyReader) this gives me benefit of having only one type and less compilation, but also mitigates performance impact on readByte.
I think it would make sense to push this pattern in std since it's common to use buffered readers anyway.

[accelbread]

I put up #22747 which bases AnyReader on GenericReader, fixing this (need to fix zig2 errors in it). Feel free to review at your convenience.

[accelbread]

@tomc1998 Do you have sample code available where you are seeing the big performance difference?

[andrewrk]

dupe of #17985 and #21566