Use Read + Seek instead of Ext4Read

[axelkar]

Thanks for the crate! Could you use a Read + Seek bound instead of the custom Ext4Read trait? Using a Read+Seek bound would allow memory-mapping (instead of read syscalls; implementing Ext4Read for &[u8] would do this too) or reading inside archives.

On no_std you could use embedded_io traits. Read Seek

Another possibility is that you keep using the Ext4Read trait, but also implement it for all Read + Seek types.

[axelkar]

Additionally types implementing AsRef<[u8]> could be used via Cursor. I'm positive Rust can optimize/monomorphize away e.g. the position field when only SeekFrom::Start is used and next to reads.

[nicholasbishop]

Thanks for filing this. Do you have code showing exactly what you'd like to do, ideally demonstrating why an impl of the existing Ext4Read trait isn't possible? The existing impl Ext4Read for File is essentially just a seek and a read. It's not obvious to me why memory-mapping can't be done with Ext4Read.

Implementing Ext4Read for &[u8] isn't that useful because we store the reader as Box<dyn Ext4Read>, so the lifetime of the reference would be restricted to 'static. I'm somewhat resistant to adding a lifetime bound to Ext4 for ergonomic reasons; it's much nicer if you can always just pass around an Ext4 and not worry about any generic params. This does come with the restriction that the reader owns the data (or shares it with Rc/Arc), however.

I could be convinced that adding lifetime bounds everywhere is worth it, but I would need to see a specific use case.

[axelkar]

Thanks for filing this. Do you have code showing exactly what you'd like to do, ideally demonstrating why an impl of the existing Ext4Read trait isn't possible?

I was thinking of reading some files from an ext4 filesystem in an archive, without decompressing the whole filesystem.

The existing impl Ext4Read for File is essentially just a seek and a read. It's not obvious to me why memory-mapping can't be done with Ext4Read.

I would have to make a newtype and do unsafeties to work around the lifetime issue you described. e.g. struct MyNewtype(&'static [u8])

Implementing Ext4Read for &[u8] isn't that useful because we store the reader as Box<dyn Ext4Read>, so the lifetime of the reference would be restricted to 'static. I'm somewhat resistant to adding a lifetime bound to Ext4 for ergonomic reasons; it's much nicer if you can always just pass around an Ext4 and not worry about any generic params. This does come with the restriction that the reader owns the data (or shares it with Rc/Arc), however.

You don't even need a lifetime bound in Ext4 when you use generics.

pub struct Ext4<R> {
  superblock: Superblock,
  block_group_descriptors: Vec<BlockGroupDescriptor>,
  // Shouldn't have a RefCell, to make this Sync.
  // Now the user can decide whether they want to make this
  // multithreaded without lifetimes by using something like `Arc<Mutex<File>>`,
  // multithreaded with lifetimes by using something like `&Mutex<File>`,
  // single-threaded with lifetimes `std::io::Cursor<&[u8]>`
  // single-threaded without lifetimes `std::io::Cursor<Vec<u8>>`
  reader: R,
}

When using generics, you should also get a performance boost, since monomorphization allows for more optimizations.

When storing Ext4 in a struct, it's great to have it own the reader so you don't have to make the struct self-referential.
In this situation, the user could provide Cursor<Mmap>, where Ext4Read would be automatically implemented since Cursor<Mmap>: Read + Seek, since Mmap: AsRef<[u8]>. There would be no lifetimes, since Ext4 would own the mmap, and everything would be cleaned up automatically.

I still see value in having the Ext4Read trait, since you can perform many optimizations there.

I think you should add a function for turning Ext4<R> back to R.

I could be convinced that adding lifetime bounds everywhere is worth it, but I would need to see a specific use case.

This way enables better performance and more customization by the library user.

[nicholasbishop]

I'm still not quite clear -- is the issue with Box<&dyn Ext4Read> that you can't make it work with this problem, or that you think the performance won't be good enough with the current interface? Those are very different problems, so I want to make sure we're addressing the right one.

I would have to make a newtype and do unsafeties to work around the lifetime issue you described. e.g. struct MyNewtype(&'static [u8])

Does it need to be a &[u8], rather than an owned value?

You don't even need a lifetime bound in Ext4 when you use generics.

I would like to avoid generics entirely in the signature, unless there's a really convincing demonstration of why we need generics. Generics makes the type more cumbersome to pass around, and I'm trying to prioritize ergonomics over squeezing out every last bit of performance.

When using generics, you should also get a performance boost, since monomorphization allows for more optimizations.

It's not always true that monomorphization makes things faster, and even when it does, the performance difference is often small.

I was thinking of reading some files from an ext4 filesystem in an archive, without decompressing the whole filesystem.

This indeed seems like a case where the performance difference from monomorphization would be trivial compared to the overall cost of decompression and IO. Amdahl's law and all.

Which compression scheme are you using? It seems like for this to work in a useful way, you'd need a compression scheme that supports random access, which I wouldn't think is generally the case.

[axelkar]

I'm still not quite clear -- is the issue with Box<&dyn Ext4Read> that you can't make it work with this problem, or that you think the performance won't be good enough with the current interface? Those are very different problems, so I want to make sure we're addressing the right one.

It's that you can't attach lifetimes to Box<dyn Ext4Read>. When you can't attach lifetimes you have to transmute to a 'static borrow and the program can become unsound or even crash with even minor programming mistakes.

I would have to make a newtype and do unsafeties to work around the lifetime issue you described. e.g. struct MyNewtype(&'static [u8])

Does it need to be a &[u8], rather than an owned value?

Yes. I can't really turn a memory range from mmap into an owned Vec<u8>. Also, I won't use Rc or Arc for trivial situations where the borrow checker can figure everything out. If I were to use Rc or Arc, I'd also need to make a newtype wrapper to be able to implement Ext4Read

You don't even need a lifetime bound in Ext4 when you use generics.

I would like to avoid generics entirely in the signature, unless there's a really convincing demonstration of why we need generics. Generics makes the type more cumbersome to pass around, and I'm trying to prioritize ergonomics over squeezing out every last bit of performance.

I understand your cause for concern, but could you do something like this:

trait Ext4Read {}
struct Ext4<R = Box<dyn Ext4Read>> {
    reader: R
}

When using generics, you should also get a performance boost, since monomorphization allows for more optimizations.

It's not always true that monomorphization makes things faster, and even when it does, the performance difference is often small.

I was thinking of reading some files from an ext4 filesystem in an archive, without decompressing the whole filesystem.

This indeed seems like a case where the performance difference from monomorphization would be trivial compared to the overall cost of decompression and IO. Amdahl's law and all.

You're right

Which compression scheme are you using? It seems like for this to work in a useful way, you'd need a compression scheme that supports random access, which I wouldn't think is generally the case.

usually chunked xz

[nicholasbishop]

Yes. I can't really turn a memory range from mmap into an owned Vec

Can you use something like https://docs.rs/memmap2/latest/memmap2/? The Mmap type wouldn't need to be passed by reference.

[axelkar]

Yes. I can't really turn a memory range from mmap into an owned Vec

Can you use something like https://docs.rs/memmap2/latest/memmap2/? The Mmap type wouldn't need to be passed by reference.

Yes. I'll close this issue for now

[nicholasbishop]

Sounds good, please feel free to reopen or create a new issue if you run into issues.

[axelkar]

Implementing Ext4Read for &[u8] isn't that useful because we store the reader as Box<dyn Ext4Read>, so the lifetime of the reference would be restricted to 'static. I'm somewhat resistant to adding a lifetime bound to Ext4 for ergonomic reasons; it's much nicer if you can always just pass around an Ext4 and not worry about any generic params. This does come with the restriction that the reader owns the data (or shares it with Rc/Arc), however.

Hi. Can you use something like the following? Then I could use all of Arc<[u8]>, Vec<u8>, Mmap, etc., without there being any lifetime/generic issues. If I'm correct, that should work with dyn Ext4Read.

impl<T: AsRef<[u8]>> Ext4Read for T {

[nicholasbishop]

That impl would create conflicts:

error[E0119]: conflicting implementations of trait `Ext4Read` for type `std::fs::File`
  --> src/reader.rs:99:1
   |
39 | impl Ext4Read for File {
   | ---------------------- first implementation here
...
99 | impl<T: AsRef<[u8]>> Ext4Read for T {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::fs::File`
   |
   = note: upstream crates may add a new impl of trait `std::convert::AsRef<[u8]>` for type `std::fs::File` in future versions

[axelkar]

File doesn't implement AsRef<[u8]>, not sure what the error means.. Maybe we need to make a wrapper struct.

[nicholasbishop]

The error is because it's a blanket impl. See this stack overflow answer for a good overview, in particular:

Be careful when adding them to your types though. Because of their extensive scope, they can easily conflict with other trait implementations that may be desirable. You can only define one blanket implementation per type (even if the constraints are non-overlapping).