new map method #12

Merged
merged 3 commits into from Apr 30, 2016

Conversation

Projects
None yet
3 participants
@llogiq
Contributor

llogiq commented Apr 25, 2016

This adds mapping functions over GenericArrays
Also improves some nits in the doc comments
And ensures panic safety of all operations

new map method
This adds mapping functions over GenericArrays
Also improves some nits in the doc comments
Ensures panic safety of all operations
@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 26, 2016

Contributor

This would fix both #8 and #11, but uses an API that was only stabilized in 1.9.0 (which is current beta).

Alas I see no way to make this work with current stable.

Contributor

llogiq commented Apr 26, 2016

This would fix both #8 and #11, but uses an API that was only stabilized in 1.9.0 (which is current beta).

Alas I see no way to make this work with current stable.

src/lib.rs
+where F: Fn(&S) -> T, N: ArrayLength<T> {
+ unsafe {
+ let mut res : GenericArray<T, N> = std::mem::uninitialized();
+ if let Err(e) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {

This comment has been minimized.

@SSheldon

SSheldon Apr 28, 2016

Should panic catching be used in normal cases? I was under the impression it was only supposed to be used at FFI boundaries.

What if you mapped the uninitialized result in another struct with a Drop impl that will only run if the array is fully constructed? Something that might look like (warning, bad pseudo code follows):

struct ArrayBuilder {
    res: Option<GenericArray<T, N>>,
    len: usize,
}

impl ArrayBuilder {
    fn new() -> Self {
        ArrayBuilder { res: Some(mem::uninitialized()), len: 0 }
    }

    fn append(item: T) {
        ptr::write(&array[len], item);
        self.len += 1;
    }

    fn finish(self) -> GenericArray {
        self.res.take()
    }
}

impl Drop for ArrayBuilder {
    fn drop(&mut self) {
        if self.len != array.len() {
            mem::forget(self.res.take());
        }
    }
}

I think an approach like that could givekn us safety without having to catch panics. But I haven't tried to compile or test any of this code :) Curious to hear what you think!

@SSheldon

SSheldon Apr 28, 2016

Should panic catching be used in normal cases? I was under the impression it was only supposed to be used at FFI boundaries.

What if you mapped the uninitialized result in another struct with a Drop impl that will only run if the array is fully constructed? Something that might look like (warning, bad pseudo code follows):

struct ArrayBuilder {
    res: Option<GenericArray<T, N>>,
    len: usize,
}

impl ArrayBuilder {
    fn new() -> Self {
        ArrayBuilder { res: Some(mem::uninitialized()), len: 0 }
    }

    fn append(item: T) {
        ptr::write(&array[len], item);
        self.len += 1;
    }

    fn finish(self) -> GenericArray {
        self.res.take()
    }
}

impl Drop for ArrayBuilder {
    fn drop(&mut self) {
        if self.len != array.len() {
            mem::forget(self.res.take());
        }
    }
}

I think an approach like that could givekn us safety without having to catch panics. But I haven't tried to compile or test any of this code :) Curious to hear what you think!

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 28, 2016

Contributor

No, that won't work, because res will already have been dropped at that point.

However I'm thinking about a similar approach.

Contributor

llogiq commented Apr 28, 2016

No, that won't work, because res will already have been dropped at that point.

However I'm thinking about a similar approach.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Apr 28, 2016

@llogiq, at what point will res already be dropped? In the drop method? I'm fairly certain a struct's members won't be dropped until after the struct's drop method is called, otherwise every drop would have to be unsafe because you'd be working with memory that was already dropped.

So, if at that point we take res and forget it, I expect that we'll prevent it from being dropped.

@llogiq, at what point will res already be dropped? In the drop method? I'm fairly certain a struct's members won't be dropped until after the struct's drop method is called, otherwise every drop would have to be unsafe because you'd be working with memory that was already dropped.

So, if at that point we take res and forget it, I expect that we'll prevent it from being dropped.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 28, 2016

Contributor

Yeah, but as I said, this requires catch_panic(_) which will involve some bookkeeping. Using transmutes to pre-leak the array will probably be even faster – and possible on stable. Give me a few minutes, I'm currently testing the approach.

Contributor

llogiq commented Apr 28, 2016

Yeah, but as I said, this requires catch_panic(_) which will involve some bookkeeping. Using transmutes to pre-leak the array will probably be even faster – and possible on stable. Give me a few minutes, I'm currently testing the approach.

Change map to the nodrop approach
This requires the (quite small) nodrop crate.
The performance is now only marginally worse than on
https://github.com/llogiq/arraymap

Also updated the typenum dependency, because older
versions crash MIR on current nightlies.
@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 28, 2016

Contributor

There it is. 😄

Contributor

llogiq commented Apr 28, 2016

There it is. 😄

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Apr 28, 2016

Sorry @llogiq, now I'm just trying to understand :)

Yeah, but as I said, this requires catch_panic(_)

How does writing a struct with our own Drop impl that tosses out data that isn't fully initialized require catch_panic? We don't need to catch any panics, our Drop impl will be called on panic. We can take advantage of that to make sure our struct's state is cleaned up before being dropped, without having to catch_panic.

In fact, we could even drop the partially initialized data; we'd know how many elements are valid, so we could drop_in_place those before forgetting the whole array.

Sorry @llogiq, now I'm just trying to understand :)

Yeah, but as I said, this requires catch_panic(_)

How does writing a struct with our own Drop impl that tosses out data that isn't fully initialized require catch_panic? We don't need to catch any panics, our Drop impl will be called on panic. We can take advantage of that to make sure our struct's state is cleaned up before being dropped, without having to catch_panic.

In fact, we could even drop the partially initialized data; we'd know how many elements are valid, so we could drop_in_place those before forgetting the whole array.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 28, 2016

Contributor

The problem with that is that dropping the already initialized data calls .drop(&mut self) on that data, which could panic itself. Yes, it's very bad form to do so, but as we already deal with a panic, we are in all probability no longer in a valid program state, so a few bytes of leaked memory is an acceptable price to pay.

Contributor

llogiq commented Apr 28, 2016

The problem with that is that dropping the already initialized data calls .drop(&mut self) on that data, which could panic itself. Yes, it's very bad form to do so, but as we already deal with a panic, we are in all probability no longer in a valid program state, so a few bytes of leaked memory is an acceptable price to pay.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Apr 29, 2016

The problem with that is that dropping the already initialized data calls .drop(&mut self) on that data, which could panic itself.

AFAIK panicking while already panicking will result in an abort, so you'd still preserve memory safety in that case :) Having trouble finding a reference on that, but the Drop docs do say:

Given that a panic! will call drop() as it unwinds, any panic! in a drop() implementation will likely abort.

The problem with that is that dropping the already initialized data calls .drop(&mut self) on that data, which could panic itself.

AFAIK panicking while already panicking will result in an abort, so you'd still preserve memory safety in that case :) Having trouble finding a reference on that, but the Drop docs do say:

Given that a panic! will call drop() as it unwinds, any panic! in a drop() implementation will likely abort.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 29, 2016

Contributor

Ah, I hadn't seen that. You're right – all it takes is a wrapper that has Drop. I may extend this PR or do another one, however you like. Note that I still think that leak amplification is the right default choice here – in the usual case, the panic unwinds all the way to the end of the program (which, in absense of catch_unwind or multithreading is the norm), and the OS will take care of all the memory.

Contributor

llogiq commented Apr 29, 2016

Ah, I hadn't seen that. You're right – all it takes is a wrapper that has Drop. I may extend this PR or do another one, however you like. Note that I still think that leak amplification is the right default choice here – in the usual case, the panic unwinds all the way to the end of the program (which, in absense of catch_unwind or multithreading is the norm), and the OS will take care of all the memory.

@SSheldon

This comment has been minimized.

Show comment
Hide comment
@SSheldon

SSheldon Apr 29, 2016

Heh I've got no say in this codebase, I just ended up here from your blog post because I was curious about different approaches :)

Nice find on NoDrop, by the way! I tried out my original approach using Option and was foiled by the null-pointer optimization. Always something fun to learn when dealing with unsafe.

Heh I've got no say in this codebase, I just ended up here from your blog post because I was curious about different approaches :)

Nice find on NoDrop, by the way! I tried out my original approach using Option and was foiled by the null-pointer optimization. Always something fun to learn when dealing with unsafe.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 29, 2016

Contributor
Contributor

llogiq commented Apr 29, 2016

@fizyk20

This comment has been minimized.

Show comment
Hide comment
@fizyk20

fizyk20 Apr 30, 2016

Owner

I had no private computer for a while, so I couldn't take a proper look at this, sorry!

This looks really cool :) If I understand correctly, the latest version uses NoDrop so that when there is a panic on initialization, the uninitialized contents of the GenericArray don't get dropped and we are still safe, is that right?

Owner

fizyk20 commented Apr 30, 2016

I had no private computer for a while, so I couldn't take a proper look at this, sorry!

This looks really cool :) If I understand correctly, the latest version uses NoDrop so that when there is a panic on initialization, the uninitialized contents of the GenericArray don't get dropped and we are still safe, is that right?

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 30, 2016

Contributor

Exactly.

Contributor

llogiq commented Apr 30, 2016

Exactly.

@fizyk20

This comment has been minimized.

Show comment
Hide comment
@fizyk20

fizyk20 Apr 30, 2016

Owner

Awesome :) I'm merging, then, thanks a lot! :)

Owner

fizyk20 commented Apr 30, 2016

Awesome :) I'm merging, then, thanks a lot! :)

@fizyk20 fizyk20 merged commit c51defd into fizyk20:master Apr 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@llogiq llogiq deleted the llogiq:map branch Apr 30, 2016

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Apr 30, 2016

Contributor

You're welcome. By the way, I'm still looking out for improvements against the NoDrop solution, so if anything comes up, I'll send it to you. 😄

Contributor

llogiq commented Apr 30, 2016

You're welcome. By the way, I'm still looking out for improvements against the NoDrop solution, so if anything comes up, I'll send it to you. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment