crossbeam-rs / crossbeam-utils Goto Github PK

Here are some hack documentations that can be dropped now that CachePadded uses repr(align(N)) on stable:

• Struct-level: "Size and alignment" section.
• new(): "Panics" section

Unit tests for Drop and Clone can also probably be dropped, as these are automatically derived in the new design.

Hello,

I'm concerned with regard to AtomicOption type. On weak architectures, consider the following:

let a = Arc::new(AtomicOption::new());
// CPU 0
// The box is visible to current CPU but not to others yet.
a.swap_box(box [0; 0xffff], Relaxed);
// CPU 1
// Pointer is now visible but the contents of box might be not.
a.take(Relaxed);

CPU 0 puts box in the AtomicOption using Relaxed order, and within a few moments, once it is visible, CPU 1 takes it. What we should end up with is CPU 1 with a pointer to potentially invalid memory, because the contents in memory that the pointer points to, which have just been written to by CPU 0 might not be visible to CPU 1 yet. I expect that with AcquireRelease barrier at swap and Acquire barrier at take, when used together with Relaxed swap itself make AtomicOption correct.

I'm by no means expert in this area, so, if I'm wrong, explanation as to why AtomicOption is correct even without barriers would be appreciated.

This crate is growing. Now it has AtomicCell, AtomicConsume, ShardedLock, CachePadded, and scoped threads. Dependencies are growing, too: ShardedLock pulls in lazy_static, parking_lot, and num_cpus as dependencies, which might be a problem for libraries depending on crossbeam-utils.

There are some libraries that only need CachePadded, or only need scoped threads, so pulling in all the other things will be too much bloat for them. It's typically not a problem for applications to have a bunch of dependencies, but libraries care a lot about being minimal. In other words, if you're developing an application, it's okay to add the whole crossbeam as a dependency, but if you're writing a library, you should choose the parts of crossbeam you need.

I suggest we split up crossbeam-utils so that libraries can choose what to depend on more precisely. My suggestion is:

• Move crossbeam_utils::thread into a new crate crossbeam-thread.
• Move crossbeam_utils::sync into a new crate crossbeam-sync (it's supposed to be for synchronization primitives like ShardedLock, WaitGroup, and things like that).
• Keep crossbeam_utils::atomic in crossbeam-utils, although we might want to move it out in the future, too.
• Keep CachePadded in crossbeam-utils.

It's unfortunate that we need to have so many crates, but it's just the nature of the project - crossbeam is really a bunch of somewhat related tools for concurrency.

cc @Vtec234

As mentioned in #27 I had a few questions about the scoped thread code, which could possibly also be suggestions for improvements, but maybe there are some subtleties involved that I don't fully understand.

1. The Scope struct uses a somewhat strange custom linked list for storing the boxed destructors/deferred closures. I am not sure why this is, I've tried replacing it with a simple Vec as well as std::collections::LinkedList and both solutions worked equally well, although I did not do any in-depth testing and panicking destructors might have unexpected side effects

2. The linked list is wrapped in a RefCell like so: dtors: RefCell<Option<DtorChain<'a, ()>>>.
   When I tried replacing the DtorChain with a regular Vec or LinkedList I also removed the RefCell and changed all relevant &Scope and &self references to mutable ones and there were no aliasing problems, so I am not sure why the RefCell wrapping exists.

3. Scope implements the Drop trait. Since drop_all() is called manually before the end of the scope function, implementing Drop should not be necessary. Also, the signature for drop_all could be changed to fn drop_all(self).

4. The implementation uses a fairly arcane procedure to allocate a Box with std::mem::ManuallyDrop and std::mem::unitialized and casts a raw pointer into an usize in order to smuggle it across thread boundaries and into the two ScopedJoinHandles.
   I thought there must be a better and more readable/understandable way to express this, but haven't figured one out yet.
   The stdlib uses a Arc<UnsafeCell<Option<T>> which is smuggled across thread boundaries.
   Not sure if using an Arc might have a negative impact on performance, but using Option<T> instead of mem::uninitialized() would be preferable I believe.

5. This is purely my own opinion but I believe the scoped thread module could benefit from a few rearrangements here and there, keeping structs and their impls together (ScopedJoinHandle is all over the place for instance) and maybe putting some things into submodules.

It is currently undocumented what happens when joining a panic-ed thread with ScopedJoinHandle::join.

std::thread::JoinHandle::join returns -> Result<T> instead of -> T, which returns the result of the panic.

BTW, I think automatically panicing is fine behavior.

Currently they return a ScopedJoinHandle<()>.

Currently having the same issue as #19, while using nightly-x86_64-pc-windows-msvc rustc 1.28.0-nightly (1d4dbf488 2018-06-11). Issue also occurs on rustc 1.26.2 (594fb253c 2018-06-01).

error[E0658]: the struct `#[repr(align(u16))]` attribute is experimental (see issue #33626)
  --> C:\Users\Kyle\.cargo\registry\src\github.com-1ecc6299db9ec823\crossbeam-utils-0.4.0\src\cache_padded.rs:19:1
   |
19 | #[repr(align(64))]
   | ^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(repr_align)] to the crate attributes to enable

error[E0658]: non-string literals in attributes, or string literals in top-level positions, are experimental (see is
  --> C:\Users\Kyle\.cargo\registry\src\github.com-1ecc6299db9ec823\crossbeam-utils-0.4.0\src\cache_padded.rs:19:1
   |
19 | #[repr(align(64))]
   | ^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(attr_literals)] to the crate attributes to enable

error: aborting due to 2 previous errors

Hello, while trying to compile gutenberg, I ran into this issue :

   Compiling cfg-if v0.1.5
   Compiling try-lock v0.1.0
   Compiling slab v0.4.1
   Compiling proc-macro2 v0.4.16
   Compiling mac v0.1.1
   Compiling log v0.4.4
   Compiling maplit v1.0.1
   Compiling crossbeam-utils v0.5.0
error: the struct `#[repr(align(u16))]` attribute is experimental (see issue #33626)
  --> /home/x/.cargo/registry/src/github.com-1ecc6299db9ec823/crossbeam-utils-0.5.0/src/cache_padded.rs:19:1
   |
19 | #[repr(align(64))]
   | ^^^^^^^^^^^^^^^^^^

error: non-string literals in attributes, or string literals in top-level positions, are experimental (see issue #34981)
  --> /home/x/.cargo/registry/src/github.com-1ecc6299db9ec823/crossbeam-utils-0.5.0/src/cache_padded.rs:19:1
   |
19 | #[repr(align(64))]
   | ^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

error: Could not compile `crossbeam-utils`.

Command used : cargo build --release
OS : Linux raspberrypi 4.14.73+ #1148 Mon Oct 1 16:41:23 BST 2018 armv6l GNU/Linux
Package : gutenberg

Thanks

#[repr(align(X))] has been stabilized in 1.24. Is there still enough motivation to keep CachePadded around? It is much simpler to just annotate your struct with #[repr(align(64))] than to deal with a wrapper type.

https://github.com/crossbeam-rs/crossbeam-utils/blob/master/src/scoped.rs#L339

This line overwrites the previous content of the Box, which was instantiated with mem::uninitialized(). If the type T has a Drop impl, it will be run on the uninitialized data.

Here is my code.
It actually works with crossbeam 0.3 but in 0.4 it fails with error in crossbeam_utils

  let ress = crossbeam::thread::scope(|scope| {
            (
                scope.spawn(move || lcam.capture_vec()),
                scope.spawn(move || rcam.capture_vec()),
            )
        });

        let frames = (
            ress.0.join().map_err(|e| format!("{:?}", e)).and_then(|r| r), // HERE I GETTING ERROR ON JOIN
            ress.1.join().map_err(|e| format!("{:?}", e)).and_then(|r| r),
        );

The stack trace

stack backtrace:
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:345:21

   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:511
   5: std::panicking::continue_panic_fmt
             at libstd/panicking.rs:426
   6: rust_begin_unwind
             at libstd/panicking.rs:337
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:92
   8: core::panicking::panic
             at libcore/panicking.rs:53
   9: <core::option::Option<T>>::unwrap
             at /checkout/src/libcore/macros.rs:20
  10: <crossbeam_utils::thread::ScopedJoinHandle<'a, T>>::join
             at /home/rumatoest/.cargo/registry/src/github.com-1ecc6299db9ec823/crossbeam-utils-0.5.0/src/thread.rs:369
  11: arcam::app::App::update
             at src/app.rs:89

It might cause UB. See here.

Currently, the borrow checker rejects this code:

thread::scope(|scope| {
    scope.spawn(|| {
        scope.spawn(|| { // ERROR: cannot borrow `scope` here
        });
    });
});

We need a special spawn method that passes that same scope into the spawned thread. Perhaps something like this:

thread::scope(|scope| {
    scope.spawn_with(|scope| {
        scope.spawn(|| {
        });
    });
});

Bikeshedding 🚲: is spawn_with a good name for this method? Any other ideas? Does scope.with_spawn() read better?

Alternatively, rather than adding a new method, perhaps we could just pass the scope into scope.spawn()? The drawback is that writing scope.spawn(|_| { ... }) might get slightly annoying, but perhaps it's not too bad?

@pedrocr You might be interested in this feature.

• Scope -------- : pub fn spawn<F, T>(&self, f: F) -> ScopedJoinHandle<T>
• ScopedThreadBuilder: fn spawn<F, T>(self, f: F) -> Result<ScopedJoinHandle<T>>

I'm not sure if there is a justification for Builder::spawn to return an io::Error but Scope::spawn to not. If there is it should be documented why this is the case and how an error can come about, if there isn't it may be worth a breaking change.

Love the library!

This is related to #6 , but not identical. The problem is to know what happens when a scoped thread panics, but the user did not explicitly join it.

We currently don't have any tests for scoped threads, while they actually contain a few subtleties. In particular, the tests should check that panics in threads are properly handled.