Use atomic crossbeam structure to add interior mutability to common::Errors

[torkleyy]

This allows fetching Errors without limiting concurrency.

[torkleyy]

cc @Xaeroxe @omni-viral @ebkalderon This should also improve performance for Amethyst in the future, and is the same strategy we should use for events in case we want to implement them as described in amethyst/amethyst#229

[zakarumych]

@torkleyy I believe that crossbeam's MsQueue will add more overhead than collecting errors that Systems return. There are two reasons.

• Dynamic allocations on each push.
• Lock-free algorithm uses loop with CAS

Of course the fact it is optional is a good thing.
But collecting Result<(), !> from System::run will be noop too.

[torkleyy]

@omni-viral I don't think so, but the bigger problem I have with Systems returning Result is that it is a breaking change that forces all users to change all of their system implementations and that you have to add Ok(()) everywhere, even though most systems don't need results. To improve ergonomics, I planned to add a method Errors::execute:

fn run(..) {
    let x = errors.execute(|| {
        let val = do_something_that_can_fail()?;
        
        val + 5
    });

    ints.insert(entity, x);
} 

Just look at these systems used in an actual game built with Specs: https://github.com/agmcleod/ld39/tree/master/src/systems

None of them needs Results and their (performance-wise and syntactical) overhead.

[torkleyy]

The difference here is also that the crossbeam structure only does something if errors get pushed, but the collecting of Results happens every frame. This means the best-case performance (no error), which is the common one, is better with this solution.

[zakarumych]

This means the best-case performance (no error), which is the common one, is better with this solution.

The good reason I often forget about 😄
I always assume worst case scenario.

[torkleyy]

@omni-viral So can I take this as an approval?

[zakarumych]

Maybe it is better to return draining iterator instead?

[torkleyy]

Crossbeam doesn't offer that, but I could still implement it in this module.

[zakarumych]

Isn't there a method to pop all queue at once?

[torkleyy]

No.

[zakarumych]

I'll take a look if it may be added 😄

[torkleyy]

@omni-viral Done.

[torkleyy]

Merging this now, as this is blocking me atm.

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push

[zakarumych]

Great!