Refactor and optimize `fetch_crate_cratesio` #180

NobodyXu · 2022-06-12T07:10:06Z

Fixed #170 .

Refactor

Split API extract_archive_stream: Create separate APIs for bin, zip and other tar-based formats:
- extract_bin
- extract_zip
- extract_tar_based_stream
- extract_tar_based_stream_and_visit
Use visitor pattern (callback) instead of the filter pattern in untar
Refactor out a new function create_tar_decoder out of extract_compressed_from_readable: Now extracter only contains 43 lines.
Replace download_and_extract_with_filter with download_tar_based_and_visit, a more versatile solution which enables fetch_crate_cratesio to be completely free of disk I/O.
~~Refactor mod drivers and extract submodules from it~~
Refactor and extract out new mod fmt for PkgFmt and add newtype PkgFmtDecomposed and TarBasedFmt.

Optimize

Optimize fetch_crate_cratesio to remove all disk I/O.
Rewrite the downloader to run in the same thread as the extracter: async_extracter now only contains 129 lines and it does not create blocking threads anymore, which has significant overhead.

Added

pub trait PathExt {
    /// Similiar to `os.path.normpath`: It does not perform
    /// any fs operation.
    fn normalize_path(&self) -> Cow<'_, Path>;
}

Signed-off-by: Jiahao XU <[email protected]>

dedicated to different tasks Signed-off-by: Jiahao XU <[email protected]>

instead of `PkgFmt` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

signature of `download_and_extract_with_filter` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

and `download_tar_based_and_visit`. By using these two items, we avoid any I/O altogether. Everything happens in memory, thus there will be no i/o related errors or cost. This commit does not regress anything because `helpers::load_manifest_path` calls `Manifest::from_path_with_metadata`, which read in the whole `Cargo.toml` file at once. Thus this commit would not cause any OOM when the the original code would not. Signed-off-by: Jiahao XU <[email protected]>

Since mod `cretesio` is the only one need to have access to it. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Rm unused param `path` and the unnecessary `fs::create_dir_all` since the tar will not be extracted to disk. Signed-off-by: Jiahao XU <[email protected]>

`path` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

It wraps a `Stream<Item = Result<Bytes, E>>` and implements `Read` and `BufRead` on it so that it can be used on sync context. Signed-off-by: Jiahao XU <[email protected]>

instead of `Result<Archive, BinstallError>` Signed-off-by: Jiahao XU <[email protected]>

so that if the `io::Error` wraps a `BinstallError`, we would just unwrap it and return the inner `BinstallError`. Otherwise, just wrap the `io::Error` in a `BinstallError`. Signed-off-by: Jiahao XU <[email protected]>

This have the following advantage: - Remove the mpsc channel, which: - Remove synchronization required for mpsc. - Remove the internal buffering of the mpsc channel, which avoid potentially OOM situation. - Improve data locality since it no longer needs to be sent over thread. - It uses `block_in_place` to avoid creating an additional blocking thread. The disadvantages would be that the downloader can no longer be run in parallel to the extracter. If the bottleneck is the decompressor, then the downloader should also pause and wait for the decompressor to consume the data. But if the bottleneck is the network, then that might be an issue. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

It was called before because `spawn_blocking` requires that, but we now switches to `block_in_place` which no longer needs this. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Use consistent codestyle for specifing trait bounds. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu · 2022-06-18T08:57:51Z

@passcod The 2rd round of optimization is done!

Now async_extracter only contains 129 lines and extracter contains 43 lines, though the github diff generates a lot of garbage diff.
The diff for driver.rs is also significantly reduced.

Feel free to review at anytime.

passcod

Superficial review only, will do a more in-depth one tonight

src/drivers/vfs.rs

src/drivers/visitor.rs

src/errors.rs

src/fmt.rs

src/helpers.rs

src/main.rs

passcod

Sorry, wrong button.

Signed-off-by: Jiahao XU <[email protected]>

Avoid one `expect`. Signed-off-by: Jiahao XU <[email protected]>

src/errors.rs

passcod · 2022-06-21T12:17:39Z

I like it, but I'll do a final pass before approvinyy and merging after a sleep.

NobodyXu added 28 commits June 11, 2022 20:15

Refactor and add new enum PkgFmtDecomposed

bd68613

Signed-off-by: Jiahao XU <[email protected]>

Refactor async_extracter: Create multi extracters

cbd57a1

dedicated to different tasks Signed-off-by: Jiahao XU <[email protected]>

Simplify download_and_extract_with_filter: Take TarBasedFmt

b1b7992

instead of `PkgFmt` Signed-off-by: Jiahao XU <[email protected]>

Update doc of download_and_extract_with_filter

d103375

Signed-off-by: Jiahao XU <[email protected]>

Rewrite AsyncExtracter: Extract fmt logic as callback fn

7b52eaa

Signed-off-by: Jiahao XU <[email protected]>

Cleanup mod async_extracter

57b40d8

Signed-off-by: Jiahao XU <[email protected]>

Simplify extract_compressed_from_readable impl

5a43ee2

Signed-off-by: Jiahao XU <[email protected]>

Rewrite untar to take a visitor & simplify

90a96ca

signature of `download_and_extract_with_filter` Signed-off-by: Jiahao XU <[email protected]>

Impl forward of &mut T to T for TarEntriesVisitor

4892d8b

Signed-off-by: Jiahao XU <[email protected]>

Impl new fn helpers::download_tar_based_and_visit

f8c8c66

Signed-off-by: Jiahao XU <[email protected]>

Refactor mod drivers: Extract out sub mods

6c6055d

Signed-off-by: Jiahao XU <[email protected]>

Set vis of find_version to pub(super)

0eb9424

Signed-off-by: Jiahao XU <[email protected]>

Add doc for TarEntriesVisitor

0162f5f

Signed-off-by: Jiahao XU <[email protected]>

Add new trait PathExt & impl for Path

cb2be5a

Signed-off-by: Jiahao XU <[email protected]>

Fix visbility of PathExt: Mark it as pub

c6687ed

Signed-off-by: Jiahao XU <[email protected]>

Impl new type Vfs which impl AbstractFilesystem

3c30722

Signed-off-by: Jiahao XU <[email protected]>

Forward impl AbstractFilesystem for &Vfs

44d4311

Signed-off-by: Jiahao XU <[email protected]>

Impl new type `ManifestVisitor

f3d3c48

Signed-off-by: Jiahao XU <[email protected]>

Mark type ManifestVisitor as pub(super)

e68eea3

Since mod `cretesio` is the only one need to have access to it. Signed-off-by: Jiahao XU <[email protected]>

Rm download_and_extract_with_filter

f82890c

Signed-off-by: Jiahao XU <[email protected]>

Mark extract_compressed_from_readable & unzip to be pub(super)

b2c3413

Signed-off-by: Jiahao XU <[email protected]>

Refactor: Simplify untar with new enum Op

17fcac7

Signed-off-by: Jiahao XU <[email protected]>

Simplify extract_tar_based_stream_and_visit

e376b71

Rm unused param `path` and the unnecessary `fs::create_dir_all` since the tar will not be extracted to disk. Signed-off-by: Jiahao XU <[email protected]>

Simplify download_tar_based_and_visit: Rm unused param

f25306f

`path` Signed-off-by: Jiahao XU <[email protected]>

Improve debug! logging in download_and_extract

e39549f

Signed-off-by: Jiahao XU <[email protected]>

Simplify fetch_crate_cratesio: Rm unused param temp_dir

24b1941

Signed-off-by: Jiahao XU <[email protected]>

Fix confusing doc of Vfs::add_path

989be49

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu mentioned this pull request Jun 12, 2022

AsyncExtracter TODOs #170

Closed

4 tasks

Update doc of PathExt::normalize_path

b879c15

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu added 12 commits June 18, 2022 17:29

Impl newtype StreamReadable

0480e99

It wraps a `Stream<Item = Result<Bytes, E>>` and implements `Read` and `BufRead` on it so that it can be used on sync context. Signed-off-by: Jiahao XU <[email protected]>

Simplify create_tar_decoder: Ret io::Result

1161a60

instead of `Result<Archive, BinstallError>` Signed-off-by: Jiahao XU <[email protected]>

Manually impl From<io::Error> for BinstallError

aba1ba7

so that if the `io::Error` wraps a `BinstallError`, we would just unwrap it and return the inner `BinstallError`. Otherwise, just wrap the `io::Error` in a `BinstallError`. Signed-off-by: Jiahao XU <[email protected]>

Rm unused items in mod helpers

621a641

Signed-off-by: Jiahao XU <[email protected]>

Rm unused trait bound in extract_impl

b152358

Signed-off-by: Jiahao XU <[email protected]>

Update doc of mod async_extracter

9de8a48

Signed-off-by: Jiahao XU <[email protected]>

Rm unnecessary to_owned call in extract_*

a5879e3

It was called before because `spawn_blocking` requires that, but we now switches to `block_in_place` which no longer needs this. Signed-off-by: Jiahao XU <[email protected]>

Revert refactor: Put find_version back to drivers

b14b711

Signed-off-by: Jiahao XU <[email protected]>

Revert refactor: Put fetch_crate_cratesio back to drivers

53bf761

Signed-off-by: Jiahao XU <[email protected]>

Fix codestyle in async_extracter

db22d7d

Use consistent codestyle for specifing trait bounds. Signed-off-by: Jiahao XU <[email protected]>

Refactor: Rm extract_impl

784a245

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu marked this pull request as ready for review June 18, 2022 08:56

passcod approved these changes Jun 21, 2022

View reviewed changes

passcod requested changes Jun 21, 2022

View reviewed changes

NobodyXu added 8 commits June 21, 2022 13:38

Add doc for Vfs

5d79af5

Signed-off-by: Jiahao XU <[email protected]>

Refactor: Extract find_version into mod

c916814

Signed-off-by: Jiahao XU <[email protected]>

Refactor: Extract fetch_crate_cratesio out into mod

b6f15f2

Signed-off-by: Jiahao XU <[email protected]>

Refactor:Mv mod visitor vfs under crates_io

23bad39

Signed-off-by: Jiahao XU <[email protected]>

Rename fmt.rs to format.rs

ad41756

Signed-off-by: Jiahao XU <[email protected]>

Refactor: Mv debug! into create_request

74a6e13

Signed-off-by: Jiahao XU <[email protected]>

Rm the duplicate debug! in main.rs:214

c5a2a89

Signed-off-by: Jiahao XU <[email protected]>

Refactor From<io::Error> for BinstallError

2f38925

Avoid one `expect`. Signed-off-by: Jiahao XU <[email protected]>

NobodyXu commented Jun 21, 2022

View reviewed changes

src/errors.rs Show resolved Hide resolved

NobodyXu mentioned this pull request Jun 21, 2022

Add new API try_downcast_inner to std::io::Error rust-lang/libs-team#57

Closed

passcod approved these changes Jun 22, 2022

View reviewed changes

passcod merged commit 3b5ea35 into cargo-bins:main Jun 22, 2022

NobodyXu deleted the refactor-and-optimize/AsyncExtracter branch June 22, 2022 03:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and optimize `fetch_crate_cratesio` #180

Refactor and optimize `fetch_crate_cratesio` #180

NobodyXu commented Jun 12, 2022 •

edited

Loading

NobodyXu commented Jun 18, 2022

passcod left a comment

passcod left a comment

passcod commented Jun 21, 2022

Refactor and optimize fetch_crate_cratesio #180

Refactor and optimize fetch_crate_cratesio #180

Conversation

NobodyXu commented Jun 12, 2022 • edited Loading

Refactor

Optimize

Added

NobodyXu commented Jun 18, 2022

passcod left a comment

Choose a reason for hiding this comment

passcod left a comment

Choose a reason for hiding this comment

passcod commented Jun 21, 2022

Refactor and optimize `fetch_crate_cratesio` #180

Refactor and optimize `fetch_crate_cratesio` #180

NobodyXu commented Jun 12, 2022 •

edited

Loading