feat: prototype async API, with demonstrable perf improvements via benchmark by Pr0methean · Pull Request #73 · zip-rs/zip2

Pr0methean · 2024-05-01T23:40:52Z

This reverts commit 1da7060.

# Conflicts: # Cargo.toml # examples/write_dir.rs # src/compression.rs # src/crc32.rs # src/lib.rs # src/read.rs # src/result.rs # src/spec.rs # src/types.rs # src/write.rs

src/tokio/combinators.rs

+    /*         } */
+
+    /*         let write_data = ready!(s.ring.poll_write(cx, s.ring.capacity())); */
+    /*         /\* FIXME: i'm pretty sure this can cause a segfault with the current */


src/tokio/combinators.rs

+    use std::{cmp, task::ready};
+
+    pub trait KnownExpanse {
+        /* TODO: make this have a parameterized Self::Index type, used e.g. with RangeInclusive or


src/tokio/write.rs

+        let fname = file.file_name.as_bytes();
+
+        if writer.is_write_vectored() {
+            /* TODO: zero-copy!! */


src/tokio/write.rs

+        let fname = file.file_name.as_bytes();
+
+        if writer.is_write_vectored() {
+            /* TODO: zero-copy!! */


src/tokio/write.rs

+        let mut name_as_string = name.into();
+        // Append a slash to the filename if it does not end with it.
+        if !name_as_string.ends_with('/') {
+            /* TODO: ends_with('\\') as well? */


src/spec.rs

+        };
+
+        if writer.is_write_vectored() {
+            /* TODO: zero-copy?? */


src/spec.rs

+        };
+
+        if writer.is_write_vectored() {
+            /* TODO: zero-copy?? */


src/tokio/read.rs

+    ) -> impl Stream<Item = ZipResult<Pin<Box<ZipFile<'_, S, Limiter<S>, Sh>>>>> + '_ {
+        let len = self.shared.len();
+        let s = std::cell::UnsafeCell::new(self);
+        /* FIXME: make this a stream with a known length! */


src/spec.rs

+        };
+
+        if writer.is_write_vectored() {
+            /* TODO: zero-copy!! */


src/tokio/read.rs

+
+    #[inline]
+    pub fn is_dir(&self) -> bool {
+        /* TODO: '\\' too? */


matthewgapp · 2024-06-12T22:29:06Z

is this still in progress @Pr0methean? Would love to see parallelization in this crate! Thanks for your work :D

Pr0methean · 2024-06-13T03:43:40Z

You'll have to ask @cosmicexplorer, since he's the PR author.

cosmicexplorer · 2024-06-13T05:03:22Z

Thanks for the ping! I think #72 is probably worth pursuing first, as it achieves parallelism without the separate effort of an async API (which via tokio includes Send and some Sync bounds). This should be easier to reliably benchmark and avoids some of the slowdown achieved by moving to async i/o, which was only regained with quite a bit of extra work in this change. When originally uploaded, this PR was largely a chance for me to learn more about async implementation, but at this point I think the amount of changes necessary ends up making this almost a completely new crate. I was ready to do that before @Pr0methean took this over, but I think making ZipArchive Send is probably sufficient to use it in most async code in a blocking thread, and (unless that's already been done in the meantime) that requires a few very small changes instead of this entire massive one.

I will definitely take a look at updating #72, since it makes use of several techniques I initially developed for pex-tool/pex#2175 (which I should also get back to) and which I don't believe have yet been implemented elsewhere. @matthewgapp let me know if synchronous parallel extraction/zipping serves your purposes -- I think an async version of that (which is what this change does) would be very cool, but it requires a lot of extra code, and is slower on all the benchmarks I've tried. If there is some user that's performing a very large number of parallel zip extractions and finds this PR to be more performant than #72, I think it might be worth taking a look at again, but I would at least want to know what performance scenario could possibly produce that, because I wasn't able to figure out how to create such a scenario on my laptop.

cosmicexplorer · 2024-06-13T14:34:02Z

The massive amount of changes required here mean I would prefer not to maintain this myself or force @Pr0methean to do so unless there is a very specific use case for which this achieves superior performance to the synchronous work in #72. In general, I'm thinking that if #72 does its job right, it should be fast enough that we don't need to rely on async, which in rust is better suited for doing less work at a time rather than doing work faster. I would love to be proven wrong here, but otherwise this work was a fun experiment and probably not suitable for production. Please anyone feel free to comment should there be any performance scenario for which async would be better suited.

matthewgapp · 2024-06-13T17:40:18Z

@cosmicexplorer sync parallel execution is just fine! We'd be using it in async code so it would have to be Send.

cosmicexplorer · 2024-06-13T18:18:48Z

Ok, great!! Now that I have @Pr0methean reviewing my work I'm thinking a full async API might not be a terrible idea at some point, but I might want to try an mmap-based implementation first. I started playing around with linux-specific splice and copy_file_range and I suspect that would accelerate the split/merge operations used for bulk creation/extraction, but mmap may be more performant for random access operations. In general the performance improvements from here on out probably require using a higher-level and less-general API (as per my research in https://github.com/cosmicexplorer/medusa-zip), and I don't expect they'd replace the standard API of this crate.

cosmicexplorer · 2024-06-13T21:45:06Z

Actually, it looks like #72 only contains the (comparatively simpler) pipelined extraction work, without the more experimental (but extremely useful) split/merged zip creation work. I've created #193 to track this work.

cosmicexplorer added 30 commits September 6, 2023 23:23

use cfg-if in a few places

d9075e8

continue smoothing with feature guards

d0355d2

add feature info for docs.rs

426a621

use num_enum to clean up the System type

dc52677

use displaydoc and thiserror to remove some boilerplate

c1e1272

make DateTime parsing and error messaging much cleaner

ff76ca2

avoid making this a breaking change and add a doctest

00c6721

finally make zip files Send

748728c

make async versions of all the types of streams

b544dc3

attempt to implement async extract

d1d83a1

impl async unit tests for crc32

de84976

get a working example of ReadAdapter?

95e0919

Limiter works now!!!

179ea98

get reader works!

0e93517

use TryStreamExt

8de2093

add criterion benchmarks that show async is slower lol

fee0cb3

ok, but at least async reading is much much faster than sync!!

24371b3

do lots of i/o copying and get very interesting results

8fc3138

mess around a little

d60985c

add tmp testproject

21a233c

make lots of changes at once oops

c9c4658

move write head stuff into ops::Drop impls

5f0cbbf

add PermitState enum for invariant checking

7faf9bd

add num_enum for easy transposition with atomics

18db885

expand Lease::AlreadyTaken

8bd8063

remove wrapper mod

eb02433

try to make the ring buffer atomic????

315f32e

allow truncating

1885103

initial impl of RingFuturized

a6d7a32

modify Read/WritePermit to remove the lifetime

239d246

cosmicexplorer and others added 13 commits October 10, 2023 03:39

add rayon dep

aea5359

tiny fix to doctest

3c2bf8d

add another rayon usage

d07a48e

fix note

87f21bb

add impl for splitting into files, but tests are broken

6843d2d

stop making Limiter impl Seek

1da7060

Revert "stop making Limiter impl Seek"

d48617c

This reverts commit 1da7060.

add back ring buf impl to prep for async fd impls

868b737

add very simple fake async impls for fd wrappers

9ae3c7d

fix tokio::io::copy test cases

329a309

impl Clone for FromGivenOffset

eda9f45

impl for MutateInnerOffset

f755697

Merge remote-tracking branch 'cosmicex/async-executor' into oldpr409

952bfc9

# Conflicts: # Cargo.toml # examples/write_dir.rs # src/compression.rs # src/crc32.rs # src/lib.rs # src/read.rs # src/result.rs # src/spec.rs # src/types.rs # src/write.rs

Pr0methean enabled auto-merge May 1, 2024 23:41

Pr0methean mentioned this pull request May 1, 2024

prototype async API, with demonstrable perf improvements via benchmark zip-rs/zip-old#409

Closed

github-advanced-security bot found potential problems May 1, 2024

View reviewed changes

Merge branch 'master' into oldpr409

049ce91

Pr0methean added the please-sign-your-commit label May 2, 2024

Pr0methean mentioned this pull request May 10, 2024

Async support using tokio for Read/Write calls #108

Open

Pr0methean added the no activity by author for 30 days label Jun 3, 2024

cosmicexplorer closed this Jun 13, 2024

auto-merge was automatically disabled June 13, 2024 14:34
Pull request was closed

cosmicexplorer mentioned this pull request Jun 13, 2024

parallel operations workstream #193

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prototype async API, with demonstrable perf improvements via benchmark#73

feat: prototype async API, with demonstrable perf improvements via benchmark#73
Pr0methean wants to merge 162 commits intomasterfrom
oldpr409

Pr0methean commented May 1, 2024

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

matthewgapp commented Jun 12, 2024 •

edited

Loading

Uh oh!

Pr0methean commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024 •

edited

Loading

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

matthewgapp commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Pr0methean commented May 1, 2024

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

matthewgapp commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pr0methean commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

matthewgapp commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

cosmicexplorer commented Jun 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthewgapp commented Jun 12, 2024 •

edited

Loading

cosmicexplorer commented Jun 13, 2024 •

edited

Loading