Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

perf: Faster cde rejection (prerequires #87) #85

Closed
wants to merge 19 commits into from
Closed

Conversation

Pr0methean
Copy link
Member

This replaces #47.

caldwell and others added 4 commits April 30, 2024 08:49
Buffer the part we need to search over (last 64K of the file) so that
we don't do a 4 byte read 65536 times.
Read large chunks of data at a time to dramatically cut down on syscalls.
Add tests for various edge cases to validate.
@Pr0methean Pr0methean enabled auto-merge May 2, 2024 19:45
@Pr0methean Pr0methean added the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label May 2, 2024
@Pr0methean
Copy link
Member Author

This cannot be merged until #87 is merged, because I rewrote it to anticipate the replacement of the byteorder crate with LittleEndianReadExt and LittleEndianWriteExt. (If I hadn't done so, this PR would block #87 from merging.)

@Pr0methean Pr0methean changed the title perf: Faster cde rejection perf: Faster cde rejection (prerequires #87) May 3, 2024
@Pr0methean Pr0methean disabled auto-merge May 3, 2024 02:37
@Pr0methean Pr0methean added waiting on another PR It won't be possible to merge this PR until another open PR is merged first. and removed waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. labels May 3, 2024
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
@Pr0methean Pr0methean enabled auto-merge May 3, 2024 17:37
@Pr0methean Pr0methean added waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. and removed waiting on another PR It won't be possible to merge this PR until another open PR is merged first. labels May 3, 2024
@Pr0methean
Copy link
Member Author

Rebased this PR because https://github.com/zip-rs/zip2/actions/runs/8945282269/job/24574475474?pr=85 wasn't going to finish before it timed out.

Pr0methean added 2 commits May 3, 2024 19:08
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
@Pr0methean
Copy link
Member Author

Closing because this causes a severe performance regression in fuzz_read.

@Pr0methean Pr0methean closed this May 4, 2024
auto-merge was automatically disabled May 4, 2024 16:06

Pull request was closed

@Pr0methean Pr0methean removed the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label May 4, 2024
@caldwell
Copy link
Contributor

Sorry I lost track of this, the pr moved several times. It's a significant improvement over the original code (in the 400x faster range)… Why didn't this get merged? Is there something I can do to help?

@Pr0methean
Copy link
Member Author

Try redeveloping and retesting it against the current mainline.

@Pr0methean Pr0methean reopened this Jun 13, 2024
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
@Pr0methean Pr0methean added the Please fix failing tests Tests are failing with this change; please fix them. label Jul 6, 2024
@Pr0methean
Copy link
Member Author

@caldwell I believe #93 renders this PR obsolete. If not, could you please open a new PR?

@Pr0methean Pr0methean closed this Jul 6, 2024
@caldwell caldwell mentioned this pull request Oct 23, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Please fix failing tests Tests are failing with this change; please fix them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants