Replace scopes with guards

[ghost]

Rendered

Implementation

Closes #19.

[jeehoonkang]

Thanks for the RFC! I'm almost approving this RFC, but I'd like to discuss the section on default_handle() a little bit more. I left two comments.

[jeehoonkang]

I'm not sure how this solves the problem discussed in crossbeam-rs/crossbeam-epoch#28. What's the benefit of this over the version in the PR? (https://github.com/crossbeam-rs/crossbeam-epoch/pull/28/files#diff-aeb58025d21f8e06a38eae37fad74b8dR26)

[ghost]

I've added a more elaborate explanation to the text.
The main benefit is that default_handle is a safe function.

[jeehoonkang]

It seems there's 1ns diff between epoch::pin() and epoch::default_handle().pin() (12ns vs. 13ns). How about specifically pointing out this benchmark result here?

[ghost]

Done.

[jeehoonkang]

"we've been"

[ghost]

Fixed.

[jeehoonkang]

If dropping Arc<Global> happens before marking Local as deleted, isn't it possible that Global is dropped so that every associated Local's are dropped, after which marking the Local as deleted is use-after-free?

[ghost]

Good point!

This is a mistake - it's actually the other way around. First, the Local is marked as deleted, and then the Arc<Global> is dropped. I'll update the text.

[jeehoonkang]

I think the options 3. and 4. are as unsafe as 1. and 2., as they may panic: https://doc.rust-lang.org/1.21.0/std/thread/struct.LocalKey.html#panics . For this reason, I think the signature of thread_local!'s HANDLE.with() is misleading: while the signature says it's safe, it is actually quite unsafe. (I'd like to cc @joshlf, who met this panic in the development of elfmalloc.)

That being said, I'd like to advocate for option 1., which doesn't incur runtime performance.

[ghost]

I've rewritten this section of the text. Hopefully it's clearer now and objectively lays out the pros & cons of each option. I've also removed the 3. option.

For this reason, I think the signature of thread_local!'s HANDLE.with() is misleading: while the signature says it's safe, it is actually quite unsafe.

Just to be clear: when you say "it's actually quite unsafe", do you mean "it's actually quite dangerous", or are you talking about "unsafe" as in "invokes undefined behavior"?

[ghost]

@joshlf Could you please take a look at the Accessing the default handle section, since it is related to elfmalloc? You've been dealing with thread-locals and tuning performance a lot, so your opinion would be very helpful here.

@jeehoonkang is advocating for the 1st option, while I want the 3rd. But in the end, I don't feel particularly strongly about it.

However, there is one thing I do strongly care about: let's avoid unsafe APIs whenever possible. In that spirit, the 2nd option would be better than the 1st one.

[ghost]

@jeehoonkang

I appreciate your concerns about the performance of the 3rd option. However, I'd like put some things into perspective first. Take a look at the following benchmarks:

#[bench]
fn pin_empty(b: &mut Bencher) {
    b.iter(|| epoch::pin());
}

#[bench]
fn default_handle_pin(b: &mut Bencher) {
    b.iter(|| epoch::default_handle().pin());
}

#[bench]
fn local_handle_pin(b: &mut Bencher) {
    let handle = epoch::default_handle();
    b.iter(|| handle.pin());
}

#[bench]
fn thread_local_handle_pin(b: &mut Bencher) {
    thread_local! {
        static MY: Handle = epoch::default_handle();
    }
    b.iter(|| MY.with(|handle| handle.pin()));
}

Results:

test default_handle_pin      ... bench:          13 ns/iter (+/- 1)
test local_handle_pin        ... bench:           9 ns/iter (+/- 1)
test pin_empty               ... bench:          12 ns/iter (+/- 1)
test thread_local_handle_pin ... bench:           9 ns/iter (+/- 0)

Looks like pinning itself costs 9 nanoseconds. Accessing the thread-local (that's LOCAL.with(...) inside epoch::pin()) adds 3 more nanoseconds. Finally, incrementing/decrementing the reference count to access the default handle costs 1 more nanosecond.

However, if you store the handle in an another thread-local (in this example, MY) and access it with MY.with(...), you don't pay the cost of reference counting. This is also faster than epoch::pin(), perhaps due to better inlining opportunities (?).

The point is, that 1 nanosecond paid on reference counting really doesn't matter at all. If you need maximum performance, it's easy to have it by creating a custom thread-local anyway.

[jeehoonkang]

@stjepang thanks for the kind explanation. Now I prefer the 3rd option. Let's go for it :)

For safety, I incorrectly assumed that both 1st and 3rd options invoke undefined behavior. However, the 3rd option seems to raise a specific panic: HANDLE is already destroyed. It's arguably less unsafe, I think.

[jeehoonkang]

Thanks for a great job!

[arthurprs]

Solid proposal. Thanks for putting all this together.