Fallback to buffered IO disk operations when not on Linux

[ExceptionallyHandsome]

Closes #88

I had to implement read_exact_at/write_all_at, and guess what? There's no cross platform API for read_at/write_at! For good reason: Unix API does not affect "seek cursor", while other's do. I don't think we care since we always specify the offset explicitly, but perhaps it's undesirable? Need your input, but I think we can live with that.

There are couple of code pieces gated under cfg which is quite ugly, but all attempts at factoring these pieces out resulted in pretentious versions of roughly the same code, so I decided this is good enough. Suggestions are welcome.

Overall, this is roughly the "working" version. "Working" because I haven't had a chance to test it. I'll proceed to it in two-three days (busy days, sorry), so this is just me presenting the design for your judgement.

(and a somewhat unrelated question: is TorrentFile really supposed be public? I just see all its methods are pub and have detailed doc comments, but the struct itself is pub(crate))

[vimpunk]

Thank you very much for this!

This is very exciting, looks very close to what I had in mind. I will look into it again this weekend, a couple of notes until then.

[vimpunk]

There might be a potential problem here for a future optimization.

While currently files are write locked when written, I think we can actually parallelize writes: because each piece maps to a disjunct portion of a file, there would be no clashes among writes.

If different pieces were written to the same file at the same time, there would be multiple calls to seek which I believe would be a race condition.

I think it's ok to leave as is for now with a note in the TorrentFile::write doc cautioning against the above.

[vimpunk]

I just realized that reads to the same file are already done concurrently, so concurrent seeks could already be an issue there?

[ExceptionallyHandsome]

True. I'll be thinking what to do, probably ditching the "generic" version and using platform specific APIs for each of the supported platforms..

[vimpunk]

I wonder if here instead of the two cfg_ifs there could be a single call to a function that takes a closure which iterates over the files that the piece overlaps with and passes the file slice of each file to the given closure.

Something like:

cfg_if! {
    if #[cfg(target_os = "linux")] {
        // convert the blocks to IO slices that the underlying
        // pwritev syscall can deal with
        let mut blocks: Vec<_> = self
            .blocks
            .values()
            .map(|b| IoVec::from_slice(&b))
            .collect();
        // the actual slice of blocks being worked on
        let mut bufs = blocks.as_mut_slice();

        // write to the portion of the files that the piece overlaps with
        self.for_each_file_slice(files, |file, file_slice| {
            // the write buffer should still contain bytes to write
            debug_assert!(!bufs.is_empty());
            debug_assert!(!bufs[0].as_slice().is_empty());

            // write to file
            // `write_*` only writes at most `slice.len` bytes of
            // `bufs` to disk and returns the portion that wasn't
            // written, which we can use to set the write buffer for the next
            // round
            bufs = file.write_vectored(file_slice, bufs)?;
        })
    } else {
        // (the same situation is in the `read` method below)
        //
        // We don't have pwritev The Convenient. Instead, we have two options:
        //
        // 1. Coalesce the slices to a continuous preallocated memory buffer and
        //    write it at once.
        // 2. Iterate through the slices and issue a separate write call for each.
        //
        // The first option implies preallocating a short-living buffer
        // and destroying it shortly after. The second option
        // implies issuing a big number of writes, and possibly disk I/O operations.
        //
        // We've chosen the first option. Piece size is commonly less than 1MB
        // which is less than the page size, so the (re)allocation should be fast enough.
        // Careful benchmarking, while necessary when the project gets to the optimization phase,
        // is something I don't feel like doing just yet, and thus left as TODO
        // and an exercise to the reader.
        //
        // (another potential optimization is to preserve the buffer between `Piece::write` calls,
        // storing it in a `thread_local!` storage, but then the function will cease to be reentrant)
        let len = self.blocks.values().map(|s|s.len()).sum();
        let mut buffer = self
            .blocks
            .values()
            .fold(Vec::with_capacity(len), |mut buf, slice| {
                buf.extend_from_slice(slice);
                buf
            });
        let mut buffer = &buffer;

        // write to the portion of the files that the piece overlaps with
        self.for_each_file_slice(files, |file, file_slice| {
            debug_assert!(!buffer.is_empty());
            buffer = file.write(file_slice, buffer)?;
        })
    }
}

The same method could possibly be used for the read routine, since the concept is the same.

[ExceptionallyHandsome]

I'll look into it in N days where N is probably less than 4. The new year was more eventful than expected (don't ask).

[vimpunk]

No rush :) (same here btw)

[vimpunk]

Haha, funnily put but I agree.

[ExceptionallyHandsome]

The comment is actually wrong: 1 MEGAbyte is bigger than page size (normally 4 KILObytes). I'll fix it.

[vimpunk]

Ah, didn't catch that. That's correct. I have a suspicion that coalescing the buffers would still be faster. Opened for later #98.

[vimpunk]

Just saw this:

is TorrentFile really supposed be public? I just see all its methods are pub and have detailed doc comments, but the struct itself is pub(crate)

It's not! I try to document everything as nicely as possible though, especially more complicated things. As noted above though, no intentions of making the type public.

[ExceptionallyHandsome]

It's not! I try to document everything as nicely as possible though, especially more complicated things. As noted above though, no intentions of making the type public.

Then do you mind if I make the methods pub(crate) instead of pub? It's kinda confusing.

[vimpunk]

@ExceptionallyHandsome I thought it was sufficient to make a type's visibility restricted without having to alter its methods' visibility, but I wasn't sure what is actually good practice in this case as I haven't found much online. Do you perhaps know?

Arguably being explicit is better so I don't actually mind.

[vimpunk]

Hi @ExceptionallyHandsome, how have you been doing?

I've been very busy and haven't had a lot of time to put more thought into this MR. Do you plan to continue it at some point? I plan to recommence developmenet of cratetorrent in the coming weeks, I just have a few more priorities to attend to first.

No rush, and absolutely no problem if you don't want to pick this up. I'm just inquiring, I understand what it's like to be busy after all. :)

[nikita-fuchs]

Hey everybody, I'm new to this project and noticed that it can't be built on Mac due to the use ofpreadv and pwritev , which doesn't exist for darwin. Is this the intent to fix this issue and if so, is there any chance there will be progress on that matter ?