search

DataDog / glommio

Glommio is a thread-per-core crate that makes writing highly parallel asynchronous applications in a thread-per-core architecture easier for rustaceans.
Other
3.12k stars 164 forks source link

Allow a ReadResult to be converted to a DmaBuffer #655

Closed vlovich closed 7 months ago

vlovich commented 7 months ago

What does this PR do?

Data read from one spot can be passed directly to write it elsewhere without any interim buffers.

Motivation

0-copy shuttling of data to disk.

Related issues

650

Additional Notes

This is a hack but it let's us write reads from files without creating an interim copy. The reason it's a hack is that it's weird to have a DmaBuffer, which represents a mutable view of data, be convertible from a ReadResult - notice the panic in attempts to access it mutably.

I would rather have a new struct called ImmutableBuffer that internally looks identical to DmaBuffer and implements From, From. This way it's impossible to do DmaBuffer::from(read_result).as_mut_slice() which panics at runtime. This would be a source code change but I think given that the functions would be generic over T: Into<ImmutableBuffer>, maybe it's OK? For now stuck to what was discussed in the RFC.

I know there's formatting errors but I want to make sure we don't want to introduce the new ImmutableBuffer struct & generic write_at to cleanup the hack.

Checklist

[X] I have added unit tests to the code I am submitting [X] My unit tests cover both failure and success scenarios [X] If applicable, I have discussed my architecture

glommer commented 7 months ago

Sorry it took me a while to get to this.

I'm sympathetic to the idea of passing buffers without copying it.

There shouldn't be anything conceptually about a DmaBuffer that says it is mutable or immutable. It it just a buffer that plays well with DMA. In that sense, I like your solution.

vlovich commented 7 months ago

Kk. Updated with formatting fixes.

glommer commented 7 months ago

ok. There are some conflicts, though.

vlovich commented 7 months ago

Yeah. Sometimes I wish each test was a separate file so that merge conflicts due to adding tests weren't a thing.

vlovich commented 7 months ago

Fixed.

glommer commented 7 months ago

still failing

vlovich commented 7 months ago

A }) line got lost resolving the conflict. Sad that I'm still having to rely on text-based language-agnostic merge resolution tools.

glommer commented 7 months ago

When I was working at RedHat in 2005 I had this idea of a patch tool that would parse the AST and show you a summary of what changed, so you could read things like "This function was moved somewhere else". Totally impossible back then. Maybe possible now ?

vlovich commented 7 months ago

Yeah, every engineer I've talked to has had the same idea about an AST-aware diff tool. We also intuitively know how hard it is which is why we don't bother trying to start companies around the idea and tackle easier problems like distributed databases :D.

Looks like SemanticMerge supports rust in some fashion although I don't know the state of it based on their website https://docs.plasticscm.com/semanticmerge I should try it out.

There's also https://semanticdiff.com/ which can't be used for semantic merging but maybe could have caught this after the fact as a way to confirm the merge resolution was done correctly.

Would be nice if tooling made this easier.