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

RFC: Vec::recycle #2802

Open
wants to merge 6 commits into
base: master
from

Conversation

@golddranks
Copy link

golddranks commented Nov 3, 2019

Add method Vec::recycle that allows safe reuse of the backing allocation of the Vec, helping avoiding allocations and de-allocations in tight loops.

let mut objects: Vec<Object<'static>> = Vec::new();      // Any lifetime will do here
while let Some(byte_chunk) = stream.next() {             // `byte_chunk` lifetime starts
    // Consumes `objects`, creating a new `Vec` `temp`.
    let mut temp: Vec<Object<'_>> = objects.recycle();   // `temp` lifetime starts

    // Zero-copy parsing; `Object`s has references to `byte_chunk`
    deserialize(byte_chunk, &mut temp)?;
    process(&temp)?;

    // "Returns" `object` back to where it was.
    objects = temp.recycle();                            // `temp` lifetime ends
}                                                        // `byte_chunk` lifetime ends

Rendered

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 3, 2019

seems similar to #2756

You can use my vec-utils crate to do this,

use vec_utils::VecExt;

let mut objects: Vec<Object> = Vec::new();                  // Any lifetime will do here

while let Some(byte_chunk) = stream.next() {                // `byte_chunk` lifetime starts
    // Consumes `objects`, creating a new `Vec` `temp`.
    let mut temp: Vec<Object> = objects.drop_and_reuse();   // `temp` lifetime starts
    // let mut temp = objects.drop_and_reuse::<Object>();   // or if you prefer

    // Zero-copy parsing; `Object`s has references to `byte_chunk`
    deserialize(byte_chunk, &mut temp)?;
    process(&temp)?;

    // "Returns" `object` back to where it was.
    objects = temp.drop_and_reuse();                        // `temp` lifetime ends
}                                                           // `byte_chunk` lifetime ends
@golddranks

This comment has been minimized.

Copy link
Author

golddranks commented Nov 3, 2019

seems similar to #2756

The difference with transmuting vectors is that this pattern doesn't transmute or reinterpret any arbitrary values, and the RCFs argues it's a safe pattern.

You can use my vec-utils crate to do this,

I didn't know about vec-utils, thanks for linking it. I'll add it to the prior work tomorrow. I also prepared the API as a crate as a companion to the RFC:
https://crates.io/crates/recycle_vec

(Ninja edits for precise quotes)

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 3, 2019

One difference between our apis is that I just cteate a new Vec if layouts don't match. This allows it to be seemlessly used in generic contexts.

@golddranks

This comment has been minimized.

Copy link
Author

golddranks commented Nov 3, 2019

That seems to be a good pattern. There is possibly room for two APIs for both behaviours. On the other hand, if we some day get compile time asserts, I'd like for it to fail in cases where I thought it wouldn't allocate but the layout doesn't match and it would be forced to.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 3, 2019

Is this the same as Vec::clear + Vec::transmute (which itself is Vec::into_raw_parts + as + Vec::from_raw_parts)?

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 4, 2019

@SimonSapin yes, this is the same as that

@golddranks

This comment has been minimized.

Copy link
Author

golddranks commented Nov 4, 2019

I updated the implementation to reflect the changes I did here: rust-lang/rust#66069 This includes using Vec::into_raw_parts and Vec::clear, and rephrasing the summary.

Some things I mentioned as unresolved questions are becoming clearer:

  • There isn't currently planned or intended support for const items to refer generic type parameters in scope. This is needed for the compile time size and alignment assertion, so even if compile time assertions stabilized, this API couldn't utilize them.
  • Already mentioned in the RFC, but my concern about compatibility with speculative future allocator APIs seems to be unfounded, because the standard library already contains APIs that reinterpret an owned type and thus deallocate it as a different type than it was allocated as.
@Lokathor

This comment has been minimized.

Copy link

Lokathor commented Nov 6, 2019

This is Vec::clear then Vec::transmute with no bound because you can't witness the previous type in a bad state? that seems neat, but probably also not RFC required, since it's just one method.

@golddranks

This comment has been minimized.

Copy link
Author

golddranks commented Nov 7, 2019

@Lokathor: The rule whether an RFC is required isn't all clear to me, so I decided to write one if not just to spell out all the considerations. There is also an open PR.

@Lokathor

This comment has been minimized.

Copy link

Lokathor commented Nov 7, 2019

An RFC doesn't hurt!

Usually they're reserved for major changes to the standard library, changes to how cargo/rustc build, and changes to the language itself (eg: adding an attribute).

I'm always happy to have folks be aware of allocations though!

@@ -205,6 +205,13 @@ directly subtyping relations between two generic types in where clauses. [1]
Thus, it was deemed to be sufficient to limit verification of the memory layout to the checking
of size and alignment of the stored type.

There is an alternative to this API, provided by https://crates.io/crates/vec_utils,
that instead of panicking on mismatching size or alignment, just allocates a new `Vec`

This comment has been minimized.

Copy link
@KrishnaSannasi

KrishnaSannasi Nov 7, 2019

vec-utils does not allocate a new Vec, it just returns Vec::new() if the layouts are incompatible

This comment has been minimized.

Copy link
@golddranks

golddranks Nov 7, 2019

Author

Ah, indeed, Vec::new() doesn't allocate.

@the8472

This comment has been minimized.

Copy link

the8472 commented Nov 9, 2019

There isn't currently planned or intended support for const items to refer generic type parameters in scope. This is needed for the compile time size and alignment assertion, so even if compile time assertions stabilized, this API couldn't utilize them.

Syntactically and semantically this is already possible. Alas, rust-lang/rust#62645 means it doesn't actually work.

@Valloric

This comment has been minimized.

Copy link

Valloric commented Nov 10, 2019

Just wanted to drop a note that I think Vec::recycle is a fantastic idea. It removes yet another thing people would use unsafe for and it's 100x simpler than dealing with custom allocators (and covers a non-trivial amount of use-cases there). @golddranks Well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.