search

image-rs / image-gif

GIF en- and decoder
Apache License 2.0
148 stars 42 forks source link

Smaller read_into_buffer #149

Closed kornelski closed 11 months ago

kornelski commented 1 year ago

#[cold] on boxing function cuts a bunch of duplicate alloc-error-handling code from error paths, and get()? optimizes better than panicking [].

fintelia commented 1 year ago

Have you benchmarked these changes? Give the if self.pass > 3 { return None; }, I'd expect that indexing into a 4 element array shouldn't have error handling regardless of whether [] or get()? was used

kornelski commented 1 year ago

I've checked assembly output.

The next one in the loop isn't getting optimized out, and I've changed both for consistency (edit: I've removed redundant check, so now both are consistent and necessary.)

kornelski commented 12 months ago

This makes extract metadata benchmark 11% faster.

kornelski commented 11 months ago

@fintelia is this OK to merge?

fintelia commented 11 months ago

I haven't had a chance to review in detail and convince myself that all the behavior is unchanged. However, I would like to see a few code comments added explaining to future readers why seemingly redundant checks are included.

kornelski commented 11 months ago

Ultimately I want to implement #144 that allows skipping decoding, and also decoding frames in parallel, building on my previous separation of LZW in #140.

I thought instead of making one massive PR I'll split the work into easy to review patches. But now I'm worried that this brings undue scrutiny to every line changed. I don't think changes in this PR or #150 are complicated or controversial, but I'm left hanging with these PRs for 2 weeks, so the approach of making small PRs isn't working for me.

fintelia commented 11 months ago

Breaking things up into small PRs helps when the resulting pieces are each clearly correct. If I can glance at the diff and see it won't break things -- without having to familiarize myself with the surrounding code or lookup anything in the relevant spec -- then I can merge right away (or click "approve" to give others a chance to comment before merging a bit later). If I have to set aside time to come back and look into those sorts of details, then the size of the PR matters less.

But stepping back, I think image-rs needs to have more people sharing the review and maintenance tasks. With the current volunteers, PR reviews sometimes taking a few weeks is pretty expected. But as you note that really isn't great contributor experience...