Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow a ReadResult to be converted to a DmaBuffer #655

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

vlovich
Copy link
Contributor

@vlovich vlovich commented Apr 18, 2024

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
Copy link
Collaborator

glommer commented Apr 24, 2024

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 vlovich force-pushed the read-result-into-dma-buffer branch from 36e588f to 78d2258 Compare April 24, 2024 17:33
@vlovich
Copy link
Contributor Author

vlovich commented Apr 24, 2024

Kk. Updated with formatting fixes.

@glommer
Copy link
Collaborator

glommer commented Apr 24, 2024

ok. There are some conflicts, though.

@vlovich vlovich force-pushed the read-result-into-dma-buffer branch from 78d2258 to 4e7c09b Compare April 24, 2024 18:36
@vlovich
Copy link
Contributor Author

vlovich commented Apr 24, 2024

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

@vlovich
Copy link
Contributor Author

vlovich commented Apr 24, 2024

Fixed.

@glommer
Copy link
Collaborator

glommer commented Apr 24, 2024

still failing

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.
@vlovich vlovich force-pushed the read-result-into-dma-buffer branch from 4e7c09b to f5bfa87 Compare April 24, 2024 19:18
@vlovich
Copy link
Contributor Author

vlovich commented Apr 24, 2024

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

@glommer
Copy link
Collaborator

glommer commented Apr 24, 2024

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 ?

@glommer glommer merged commit 85b115d into DataDog:master Apr 24, 2024
5 checks passed
@vlovich vlovich deleted the read-result-into-dma-buffer branch April 24, 2024 19:56
@vlovich
Copy link
Contributor Author

vlovich commented Apr 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants