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

Remove RC from certain Owned Value Types and look for DX friendly ways to implement Zerocopy Result Row #974

Closed
wants to merge 2 commits into from

Conversation

pedrocarlo
Copy link
Contributor

@pedrocarlo pedrocarlo commented Feb 10, 2025

Exploration PR to look for ways to increase performance with hopefully minimal use of lifetimes

@penberg
Copy link
Collaborator

penberg commented Feb 11, 2025

Hey @pedrocarlo! It's not clear to me what you're planning to do here. The Rc there is to avoid expensive copies of the values, which is important in cases where OwnedValue owns the values (SQLite calls them dynamic values). However, for zero copy what we need is a way to represent values that are not owned by OwnedValue (terrible name now) and instead are just pointers to something else (called static values in SQLite IIRC).

@pedrocarlo
Copy link
Contributor Author

Hey @penberg! So this is PR is still a bit in the exploration phase. As you mentioned in #906, we should try teaching the code "about blobs and strings that are managed by someone else". The first thought I had in mind was maybe using a Cow so that clones are minimized, and it would clearly indicate if the value is owned or not. But, Im still trying to figure out if this is the right way, that is why there is no commit yet regarding some Cow implementation. However, I do not think this addresses our issue. I'm still a bit unexperienced with Rust so I am still searching for ways to do it. If you have any ideas I am all ears!

@pedrocarlo pedrocarlo closed this Feb 14, 2025
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