Faster u32 parsing #428
Merged
Faster u32 parsing #428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anforowicz
force-pushed
the
faster-u32-parsing
branch
from
b9eb5fa to
92a7e4f
Compare
This way removes a separate `State::Signature` variant and handles the
PNG signature via two `State::U32` states. This is desirable because:
* This helps to reuse the performance gains for `State::U32`
(i.e. avoid accumulating the signature byte-by-byte if the input
buffer already has all its bytes)
* Avoiding separate code also has additional benefits:
- Less pressure on the instructions cache
- Ability to reuse branch prediction for `U32`-related code
The modified code already has reasonable test coverage via
http://www.schaik.com/pngsuite/#corrupted
anforowicz
force-pushed
the
faster-u32-parsing
branch
from
92a7e4f to
d51188f
Compare
|
@fintelia, can you PTAL? |
|
👍 This change looks good to me, but I haven't yet had a chance to do a detailed read through. Hopefully should get to that soon |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series of 5 commits refactors how the code parses
u32values. The cumulative impact on the runtime of thedecode/generated-png:noncompressed-8x8.pngbenchmark looks as follows:I expect that this PR will have minimal impact on bigger images, so I didn't even try running the other benchmarks (I assume that any changes observed in the other benchmarks would be just statistical noise).
I think that this PR turned out quite nicely from code simplification and readability perspective. Even if there was no performance impact, I think it might be desirable to land it from this perspective.
Various notes:
u32values.decoder::tests::no_data_dup_on_finish- only one test... :-(BufReadchanges land, maybe we should tweak therender_imagestest to work withBufReader::with_capacity(1, <original input>)which should test dripping the input byte-by-byte.BufReader::with_capacitycalls insrc/decoder/mod.rsso that a buffer of only 1 byte was used.U32ValueKind::Typeafter anIDATchunk really needs to go through 2 state transitions (with a detour viaDecoded::ImageDataFlushedduring whichself.stateis set to the old/unchanged value). This is probably in the unnecessary-microoptimizations territory, but I was hoping to simplify the code a bit more. Still... I don't see how to do this cleanly.