Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Impl reader+writer seek #360

Merged
merged 11 commits into from
Sep 4, 2024
Merged

Impl reader+writer seek #360

merged 11 commits into from
Sep 4, 2024

Conversation

wcampbell0x2a
Copy link
Collaborator

@wcampbell0x2a wcampbell0x2a commented Sep 4, 2023 •
edited
Loading

  • Add Seek to Reader and Writer
  • Add as_mut to Reader, and tests
  • Add: seek_rewind, seek_from_current, seek_from_end, seek_from_start,

See #105

Currently being used in https://github.com/wcampbell0x2a/cpio-deku

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

Benchmark for c05f1ba

Click to view benchmark
Test Base PR %
deku_read_bits 784.1±1.34ns 784.1±1.36ns 0.00%
deku_read_byte 23.5±0.11ns 23.5±0.08ns 0.00%
deku_read_enum 10.1±0.08ns 10.1±0.07ns 0.00%
deku_read_vec 65.4±0.34ns 68.2±0.77ns +4.28%
deku_write_bits 105.5±0.39ns 105.9±1.16ns +0.38%
deku_write_byte 164.3±0.91ns 163.5±0.76ns -0.49%
deku_write_enum 105.8±0.35ns 105.8±0.31ns 0.00%
deku_write_vec 4.0±0.01µs 4.0±0.01µs 0.00%

@wcampbell0x2a wcampbell0x2a force-pushed the impl-reader branch 5 times, most recently from 9e6aca1 to bf6defb Compare September 8, 2023 01:37
@wcampbell0x2a wcampbell0x2a force-pushed the impl-reader branch 2 times, most recently from 1d23236 to c0f9fd4 Compare November 19, 2023 02:57
Base automatically changed from impl-reader to master December 14, 2023 01:53
@wcampbell0x2a wcampbell0x2a changed the base branch from master to impl-writer December 20, 2023 05:00
@github-actions GitHub Actions
Copy link

Benchmark for f9655f8

Click to view benchmark
Test Base PR %
deku_read_bits 1253.9±14.83ns 1293.6±23.46ns +3.17%
deku_read_byte 21.8±0.59ns 21.8±0.54ns 0.00%
deku_read_enum 9.5±0.16ns 9.4±0.15ns -1.05%
deku_read_vec 59.5±0.65ns 58.6±0.82ns -1.51%
deku_write_bits 142.6±5.32ns 225.8±16.05ns +58.35%
deku_write_byte 142.1±4.61ns 20.9±0.36ns -85.29%
deku_write_enum 84.4±2.43ns 19.6±0.33ns -76.78%
deku_write_vec 3.3±0.07µs 300.4±4.12ns -90.90%

@wcampbell0x2a wcampbell0x2a force-pushed the impl-writer branch 9 times, most recently from 7297dc4 to 26c00d3 Compare December 28, 2023 23:07
@github-actions GitHub Actions
Copy link

Benchmark for 84fd5f3

Click to view benchmark
Test Base PR %
deku_read_bits 1116.3±18.52ns 1197.5±16.33ns +7.27%
deku_read_byte 21.6±0.49ns 22.2±1.48ns +2.78%
deku_read_enum 9.3±0.15ns 9.3±0.13ns 0.00%
deku_read_vec 53.5±0.59ns 53.9±0.51ns +0.75%
deku_write_bits 180.7±3.25ns 188.7±21.84ns +4.43%
deku_write_byte 21.2±2.63ns 20.8±0.23ns -1.89%
deku_write_enum 20.2±0.36ns 20.1±0.18ns -0.50%
deku_write_vec 298.2±5.20ns 296.1±3.47ns -0.70%

Base automatically changed from impl-writer to master April 29, 2024 22:03
@wcampbell0x2a wcampbell0x2a force-pushed the impl-reader-seek branch 2 times, most recently from e9143bb to 0138356 Compare May 4, 2024 16:45
@github-actions GitHub Actions
Copy link

github-actions bot commented May 4, 2024

Benchmark for e6c5a67

Click to view benchmark
Test Base PR %
deku_read_bits 1229.2±16.47ns 1261.1±14.49ns +2.60%
deku_read_byte 3.3±0.12ns 3.3±0.08ns 0.00%
deku_read_enum 2.6±0.07ns 2.6±0.04ns 0.00%
deku_read_vec 33.7±0.47ns 33.4±0.33ns -0.89%
deku_write_bits 149.5±4.52ns 149.0±2.81ns -0.33%
deku_write_byte 21.8±0.51ns 22.0±0.31ns +0.92%
deku_write_enum 21.1±0.31ns 21.4±0.26ns +1.42%
deku_write_vec 351.9±3.46ns 405.2±5.23ns +15.15%

@vhdirk
Copy link
Contributor

vhdirk commented May 31, 2024

This would be pretty useful in my project. Anything I can assist with to get this merged?

@wcampbell0x2a wcampbell0x2a changed the title DRAFT: Impl reader+writer seek Impl reader+writer seek May 31, 2024
@wcampbell0x2a
Copy link
Collaborator Author

This would be pretty useful in my project. Anything I can assist with to get this merged?

We need no-std-io/no-std-io#5 and a final review by me and @sharksforarms

@wcampbell0x2a wcampbell0x2a added this to the 0.18.0 milestone May 31, 2024
@wcampbell0x2a wcampbell0x2a mentioned this pull request Jul 14, 2024
Closed
@wcampbell0x2a
Copy link
Collaborator Author

wcampbell0x2a commented Sep 2, 2024 •
edited
Loading

This would be pretty useful in my project. Anything I can assist with to get this merged?

We need no-std-io/no-std-io#5 and a final review by me and @sharksforarms

Decided to just fork that repo and use my own no-std-io2 for now, since that wasn't getting merged and i've tried contacting them a couple of times.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024

Benchmark for 830408c

Click to view benchmark
Test Base PR %
deku_read_bits 1157.1±16.19ns 1197.3±10.14ns +3.47%
deku_read_byte 3.3±0.15ns 3.3±0.02ns 0.00%
deku_read_enum 2.5±0.08ns 2.6±0.06ns +4.00%
deku_read_vec 33.7±0.21ns 34.5±0.25ns +2.37%
deku_write_bits 187.9±4.96ns 181.1±8.26ns -3.62%
deku_write_byte 22.7±0.43ns 25.2±0.26ns +11.01%
deku_write_enum 21.7±0.30ns 23.0±0.19ns +5.99%
deku_write_vec 387.4±3.96ns 404.9±4.90ns +4.52%

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024

Benchmark for fe7abff

Click to view benchmark
Test Base PR %
deku_read_bits 1185.2±9.81ns 1176.5±15.95ns -0.73%
deku_read_byte 3.3±0.04ns 3.3±0.09ns 0.00%
deku_read_enum 2.5±0.05ns 2.6±0.05ns +4.00%
deku_read_vec 33.7±0.39ns 33.9±0.49ns +0.59%
deku_write_bits 181.5±7.18ns 181.2±3.77ns -0.17%
deku_write_byte 22.7±0.33ns 25.2±0.58ns +11.01%
deku_write_enum 21.7±0.39ns 22.7±0.36ns +4.61%
deku_write_vec 390.7±3.67ns 431.1±6.61ns +10.34%

@wcampbell0x2a wcampbell0x2a mentioned this pull request Sep 3, 2024
Merged
sharksforarms
sharksforarms approved these changes Sep 3, 2024
View reviewed changes
Copy link
Owner

@sharksforarms sharksforarms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is great. This got me thinking, it would be pretty cool if there was a way to add attributes like this without adding bounds to the generic, such that "seek_" could only be used if T is Seek, else it would be a compile error. This way, we don't impose more bounds than necessary?

The downside of this PR is that now types are now required to be Seek (maybe less of a concern with the reader interface... but I can for-see some folks not liking this, maybe?)

@wcampbell0x2a
Copy link
Collaborator Author

Thanks! This is great. This got me thinking, it would be pretty cool if there was a way to add attributes like this without adding bounds to the generic, such that "seek_" could only be used if T is Seek, else it would be a compile error. This way, we don't impose more bounds than necessary?

The downside of this PR is that now types are now required to be Seek (maybe less of a concern with the reader interface... but I can for-see some folks not liking this, maybe?)

I'm not sure about any solutions that includes generic bounds, I think you'll always need to have Seek as a bounds on Reader, unless i'm wrong.

the friends at binrw 'solve' this by having a NoSeek

@wcampbell0x2a wcampbell0x2a merged commit 3337b4d into master Sep 4, 2024
11 of 15 checks passed
@wcampbell0x2a wcampbell0x2a deleted the impl-reader-seek branch September 4, 2024 02:50
wcampbell0x2a added a commit that referenced this pull request Sep 4, 2024
@wcampbell0x2a
Impl reader+writer seek (#360)
ac40406 
* Add Seek requirement to Reader

* Add basic seek attributes

* Add reader.as_mut()

* Add seek_rewind, seek_from_end, and seek_from_start

* Add seek docs

* Add to changelog

* Add seek_rewind docs

* Add compile test for seek conflict

* Add Seek to writer

* Update to using fork of no_std_io, no_std_io2

* Use fork since no-std-io/no-std-io#5
  was not getting merged in a timely fashion. This fork is maintained
  by myself.

* Move changelog entry to top
wcampbell0x2a added a commit that referenced this pull request Sep 4, 2024
@wcampbell0x2a
Impl reader+writer seek (#360)
20f1a88 
* Add Seek requirement to Reader

* Add basic seek attributes

* Add reader.as_mut()

* Add seek_rewind, seek_from_end, and seek_from_start

* Add seek docs

* Add to changelog

* Add seek_rewind docs

* Add compile test for seek conflict

* Add Seek to writer

* Update to using fork of no_std_io, no_std_io2

* Use fork since no-std-io/no-std-io#5
  was not getting merged in a timely fashion. This fork is maintained
  by myself.

* Move changelog entry to top
@sharksforarms
Copy link
Owner

the friends at binrw 'solve' this by having a NoSeek

Ah! Indeed. It was likely enough of a pain point to include something in the library.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@sharksforarms sharksforarms sharksforarms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wcampbell0x2a @vhdirk @sharksforarms