shared - i need it to be useful

[Manu]

On Thu, Oct 18, 2018 at 6:40 AM Steven Schveighoffer via Digitalmars-d
<digitalmars-d at puremagic.com> wrote:
>
> On 10/17/18 10:26 PM, Manu wrote:
> > On Wed, Oct 17, 2018 at 6:50 PM Steven Schveighoffer via Digitalmars-d
> >>
> >> The implicit cast means that you have to look at more than just your
> >> method. You have to look at the entire module, and figure out all the
> >> interactions, to see if the thread safe method actually is thread safe.
> >> That's programming by convention, and fully trusting the programmer.
> >
> > I don't understand... how can the outer context affect the
> > threadsafety of a properly encapsulated thing?
>
> [snip]
>
> > You need to take it for an intellectual spin. Show me how it's corrupt
> > rather than just presenting discomfort with the idea in theory.
> > You're addicted to some concepts that you've carried around for a long
> > time. There is no value in requiring casts, they're just a funky
> > smell, and force the user to perform potentially unsafe manual
> > conversions, or interactions that they don't understand.
>
> For example (your example):
>
> struct NotThreadsafe
> {
>    private int x;
>    void local()
>    {
>      ++x; // <- invalidates the method below, you violate the other
> function's `shared` promise
>    }
>    void notThreadsafe() shared
>    {
>      atomicIncrement(&x);
>    }
> }
>
> First, note the comment. I can't look ONLY at the implementation of
> "notThreadSafe" (assuming the function name is less of a giveaway) in
> order to guarantee that it's actually thread safe. I have to look at the
> WHOLE MODULE. Anything could potentially do what local() does. I added
> private to x to at least give the appearance of thread safety.
>
> But on top of that, if I can't implicitly cast mutable to shared, then
> this ACTUALLY IS thread safe, as long as all the casting in the module
> is sound (easy to search and verify), and hopefully all the casting is
> encapsulated in primitives like you have written. Because someone on the
> outside would have to cast a mutable item into a shared item, and this
> puts the responsibility on them to make sure it works.
>
> I'm ALL FOR having shared be completely unusable as-is unless you cast
> (thanks for confirming what I suspected in your last post). It's the
> implicit casting which I think makes things way more difficult, and
> completely undercuts the utility of the compiler's mechanical checking.
>
> And on top of that, I WANT that implementation. If I know something is
> not shared, why would I ever want to use atomics on it? I don't like
> needlessly throwing away performance. This is how I would write it:
>
> struct ThreadSafe
> {
>     private int x;
>     void increment()
>     {
>        ++x; // I know this is not shared, so no reason to use atomics
>     }
>     void increment() shared
>     {
>        atomicIncrement(&x); // use atomics, to avoid races
>     }
> }
>
> The beauty of shared not being implicitly castable, is it allows you to
> focus on the implementation at hand, with the knowledge that nothing
> else can meddle with it. The goal of mechanical checking should be to
> narrow the focus of what needs to be proven correct.
>
> -Steve

I understand your argument, and I used to think this too... but I
concluded differently for 1 simple reason: usability.
I have demonstrated these usability considerations in production. I am
confident it's the right balance.

I propose:
 1. Normal people don't write thread-safety, a very small number of
unusual people do this. I feel very good about biasing 100% of the
cognitive load INSIDE the shared method. This means the expert, and
ONLY the expert, must make decisions about thread-safety
implementation.
 2. Implicit conversion allows users to safely interact with safe
things without doing unsafe casts. I think it's a complete design fail
if you expect any user anywhere to perform an unsafe cast to call a
perfectly thread-safe function. The user might not properly understand
their obligations.
 3. The practical result of the above is, any complexity relating to
safety is completely owned by the threadsafe author, and not cascaded
to the user. You can't expect users to understand, and make correct
decisions about threadsafety. Safety should be default position.

I recognise the potential loss of an unsafe optimised thread-local path.
1. This truly isn't a big deal. If this is really hurting you, you
will notice on the profiler, and deploy a thread-exclusive path
assuming the context supports it.
2. I will trade that for confidence in safe interaction every day of
the week. Safety is the right default position here.
2. You just need to make the unsafe thread-exclusive variant explicit, eg:

> struct ThreadSafe
> {
>     private int x;
>     void unsafeIncrement() // <- make it explicit
>     {
>        ++x; // User has asserted that no sharing is possible, no reason to use atomics
>     }
>     void increment() shared
>     {
>        atomicIncrement(&x); // object may be shared
>     }
> }

I think this is quiet a reasonable and clearly documented compromise.
I think absolutely-reliably-threadsafe-by-default is the right default
position. And if you want to accept unsafe operations for optimsation
circumstances, then you're welcome to deploy that in your code as you
see fit.

If the machinery is not a library for distribution and local to your
application, and you know for certain that your context is such that
thread-local and shared are mutually exclusive, then you're free to
make the unshared overload not-threadsafe; you can do this because you
know your application context.
You just shouldn't make widely distributed tooling this way.

I will indeed do this myself in some cases, because I know those facts
about my application.
But I wouldn't compromise the default design of shared for this
optimisation potential... deliberately deployed optimisation is okay
to be unsafe when taken in context.