Impl from_reader
#352
Merged
Impl from_reader
#352
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
Benchmark for 306a2aaClick to view benchmark
|
Benchmark for 83b4dcfClick to view benchmark
|
Benchmark for 4837616Click to view benchmark
|
Benchmark for 07b37eeClick to view benchmark
|
|
Very excited about this! |
Benchmark for 6dd27b2Click to view benchmark
|
Benchmark for 1854139Click to view benchmark
|
Benchmark for 0b38ff1Click to view benchmark
|
Benchmark for 7908460Click to view benchmark
|
Benchmark for 025b256Click to view benchmark
|
Benchmark for 7c1a466Click to view benchmark
|
Benchmark for 8f4785fClick to view benchmark
|
Benchmark for ebef2caClick to view benchmark
|
Benchmark for b35b053Click to view benchmark
|
Benchmark for 3dafcceClick to view benchmark
|
wcampbell0x2a
force-pushed
the
impl-reader
branch
from
6700307 to
80be0ec
Compare
Benchmark for 76c7503Click to view benchmark
|
Benchmark for 0682edeClick to view benchmark
|
Benchmark for 5ece58fClick to view benchmark
|
wcampbell0x2a
force-pushed
the
impl-reader
branch
3 times, most recently
from
7254fb9 to
218bdca
Compare
Benchmark for 556bc8aClick to view benchmark
|
Benchmark for 2fcd848Click to view benchmark
|
Benchmark for 941c52dClick to view benchmark
|
wcampbell0x2a
force-pushed
the
impl-reader
branch
from
411acd6 to
5d4a8b2
Compare
Benchmark for 15dc49eClick to view benchmark
|
Benchmark for 42a62d8Click to view benchmark
|
wcampbell0x2a
force-pushed
the
impl-reader
branch
from
0d80be6 to
74cab11
Compare
Benchmark for a344877Click to view benchmark
|
Benchmark for ac19ec7Click to view benchmark
|
The documentation here relies more on your own opinions. Should I replace all the docs that I replaced back with I think in |
Yeah I think having an example for both makes sense! |
Benchmark for aae5406Click to view benchmark
|
sharksforarms
previously approved these changes
Oct 12, 2023
There was a problem hiding this comment.
Thanks @wcampbell0x2a ! Sorry for the delayed review, I don't have too much time for open source these days. Feel free to merge and @ me for more reviews!
Benchmark for af456aeClick to view benchmark
|
@sharksforarms make some last changes, should be ready to merge |
sharksforarms
previously approved these changes
Nov 16, 2023
sharksforarms
force-pushed
the
impl-reader
branch
from
b32ef5d to
934b42c
Compare
sharksforarms
previously approved these changes
Nov 16, 2023
Sweet. I'll squash and make a nice commit msg then merge. |
wcampbell0x2a
commented
Nov 19, 2023
wcampbell0x2a
commented
Nov 19, 2023
wcampbell0x2a
force-pushed
the
impl-reader
branch
2 times, most recently
from
1d23236 to
c0f9fd4
Compare
Add from_reader and use Read internally to improve the codegen of deku, the performance of derived parsers. Internally, this is implemented using the new src/reader.rs interface in deku parses. This has a waterfall effect on DekuReader implementations and the deku-derive/deku_read Derived impl. Previous usage with from_bytes and other API's are unchanged. There is somewhat of a performance hit for bit-only parses, but I find the major improvments in the bytes-wise parsing to be great enough to warrent this change. The following are some sample benchmarks: > critcmp before after group after before ----- ----- ------ deku_read_bits 1.24 845.8±14.29ns ? ?/sec 1.00 679.5±11.83ns ? ?/sec deku_read_byte 1.00 17.8±0.25ns ? ?/sec 2.12 37.8±3.35ns ? ?/sec deku_read_enum 1.00 15.3±0.15ns ? ?/sec 2.04 31.2±0.81ns ? ?/sec deku_read_vec 1.00 676.8±7.04ns ? ?/sec 2.16 1459.5±40.22ns ? ?/sec deku_write_bits 1.00 125.3±3.10ns ? ?/sec 1.04 130.2±11.12ns ? ?/sec deku_write_byte 1.00 161.6±4.86ns ? ?/sec 1.02 165.0±5.91ns ? ?/sec deku_write_enum 1.00 105.6±1.06ns ? ?/sec 1.03 109.0±7.20ns ? ?/sec deku_write_vec 1.00 4.6±0.04µs ? ?/sec 1.06 4.9±0.07µs ? ?/sec The above change removes DekuRead, and replaces it with DekuReader. This contains the from_reader_with_ctx. DekuContainerRead contains from_reader. The crate no_std_io was picked to supply a Read impl for the no_std feature. These are "re-export"ed. Add "`Read` enabled" docs to lib.rs Add tests/test_tuple.rs tests Update CHANGELOG.md to reflect changes and help migration to this usage Use llvm-cov in ci for the generation of more accurate coverage reports Update benchmarks to test more complex parser speeds Disable Miri CI Update ensure_no_std to work with new Read usage. Remove wee-alloc in favour of an updated crate for the allocator. Add inline to small functions
wcampbell0x2a
force-pushed
the
impl-reader
branch
from
c0f9fd4 to
cc12d18
Compare
|
@sharksforarms can only merge with 1 approving review |
sharksforarms
approved these changes
Dec 13, 2023
This was referenced Dec 27, 2023
Closed
Closed
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.
Builds upon #345 and #342. Truly fixing #44 and #344. And #252, #105. And somewhat #290.
Ok that's enough. This adds
from_readerwhich can take in a Read! I also went ahead and implemented some long standing issues with this crates and actually make aContainerthat can properly specialize on aligned byte reads.There's more when I have time to review, many TODOs are still standing. But the benchmarks look good!
benchmarks
I updated the benchmarks, so you can't just compare against master:
todo
from_readerto read from file--engine = llvmon tarpulin might produce more accurate coverage results? Update: still inaccurate. /~https://github.com/taiki-e/cargo-llvm-cov seems to be more accurate.cargo +nightly llvm-cov --doctests --release --html --ignore-filename-regex attributesproduces very good results.read(..)forno_stdenviroments#[deku(cond = "!deku::rest.is_empty()")]not working. not sure the solution on this since there is no way of knowing ifReadis done except trying to read [u8; 1] and seeing it it errors and buffer the resultsExample usage