June 11, 2012
On Fri, 08 Jun 2012 22:01:41 -0400, mta`chrono <chrono@mta-international.net> wrote:

> Would this be legal?
>
> class A
> {
>      private static shared int counter; // shared across all instances
>
>      this()
>      {
>           auto i = ++counter;
>           pragma(msg, typeof(i)); // prints int
>      }
> }
>

Yes, but it would not automatically make ++counter atomic.

My proposal is simply to avoid sharing data that shouldn't be shared (i.e. stack data and temporaries), not to make all operations thread-safe.

-Steve
June 11, 2012
On Sat, 09 Jun 2012 08:31:01 -0400, Mike Wey <mike-wey@example.com> wrote:

> On 06/09/2012 04:01 AM, mta`chrono wrote:
>> Would this be legal?
>>
>> class A
>> {
>>       private static shared int counter; // shared across all instances
>>
>>       this()
>>       {
>>            auto i = ++counter;
>>            pragma(msg, typeof(i)); // prints int
>>       }
>> }
>>
>
> Would it also be legal if the variable wasn't static?

No.

> int opApply(int delegate(ref Pixels) dg)
> {
>      shared(int) progress;
>
>      foreach ( row; taskPool.parallel(iota(extent.y, extent.y + extent.height)) )
>      {
>          int result = dg(Pixels(image, extent.x, row, extent.width, 1));
>
>          if ( result )
>               return result;
>
>          if ( image.monitor !is null )
>          {
>              atomicOp!"+="(progress, 1);
>              image.monitor()("ImageView/" ~ image.filename, progress, extent.height);
>          }
>      }
>      return 0;
> }

AFAIK, if you removed shared from progress, it would work.  I don't think std.parallel is as strict as std.concurrency (and for pretty good reason).

I think a better way to mark progress is to make it an atomic integer type (like Artur has developed).

-Steve
June 11, 2012
On Sun, 10 Jun 2012 10:22:50 -0400, Artur Skawina <art.08.09@gmail.com> wrote:

> On 06/10/12 10:51, Mehrdad wrote:
>> On Friday, 8 June 2012 at 04:03:08 UTC, Steven Schveighoffer wrote:
>>> I agree that the type should be shared(int), but the type should not transfer to function calls or auto, it should be sticky to the particular variable.
>>
>>
>> I think that's a pretty fundamental problem:
>>
>> Shared is a type constructor, but you just mentioned that it has to be a property of the _variable_, i.e. a storage class.
>
> Actually, no. What Steven is saying is that a _copy_ of the variable (the result
> of an expression, in general) does not need to retain the same qualifiers, such
> as 'shared' or 'const'. It means that the result does have a different type, but
> that's ok, as it's a different entity.

Yes, this is correct.

> This is obviously fine:
>
>    const int[8] a; auto p = &a[0]; // typeof(p)==const(int)*
>
> as we just created the pointer, there's no need for it to be const, it just needs
> to point to 'const'.
>
> Now consider:
>
>    const int*[8] a; auto p = a[0]; // typeof(p)==const(int)*
>
> Here we *also* create a new pointer, so it does not need to be const either. The
> only difference is in how the *value* of this new pointer is obtained. In the
> 'const' case the data (pointer value) can just be read from the memory location
> and the operation is always perfectly safe.

The worst is this one:

const int[8] a;  auto e = a[0]; // typeof(p) == const(int) (in current compiler)

> The reason I don't want this to happen when dealing with 'shared' is not that it's
> wrong, it isn't. It's because it would make writing unsafe/confusing/buggy code
> too easy, as you then can completely ignore the 'shared' aspect.

I wholly disagree.  In fact, keeping the full qualifier intact *enforces* incorrect code, because you are forcing shared semantics on literally unshared data.

Never would this start ignoring shared on data that is truly shared.  This is why I don't really get your argument.

If you could perhaps explain with an example, it might be helpful.

-Steve
June 11, 2012
On 06/11/12 12:26, Steven Schveighoffer wrote:
> On Sat, 09 Jun 2012 08:31:01 -0400, Mike Wey <mike-wey@example.com> wrote:
> 
>> On 06/09/2012 04:01 AM, mta`chrono wrote:
>>> Would this be legal?
>>>
>>> class A
>>> {
>>>       private static shared int counter; // shared across all instances
>>>
>>>       this()
>>>       {
>>>            auto i = ++counter;
>>>            pragma(msg, typeof(i)); // prints int
>>>       }
>>> }
>>>
>>
>> Would it also be legal if the variable wasn't static?
> 
> No.

Why? What if this class would like to launch a few threads to do some work, export the address of the counter and have them report back by updating it?

Unlike the shared-on-stack case, this wouldn't even be unsafe (the memory won't be freed until all threads stop using it.) The alternative is to have to split the class into two, more heap allocations etc.


>> int opApply(int delegate(ref Pixels) dg)
>> {
>>      shared(int) progress;
>>
>>      foreach ( row; taskPool.parallel(iota(extent.y, extent.y + extent.height)) )
>>      {
>>          int result = dg(Pixels(image, extent.x, row, extent.width, 1));
>>
>>          if ( result )
>>               return result;
>>
>>          if ( image.monitor !is null )
>>          {
>>              atomicOp!"+="(progress, 1);
>>              image.monitor()("ImageView/" ~ image.filename, progress, extent.height);
>>          }
>>      }
>>      return 0;
>> }
> 
> AFAIK, if you removed shared from progress, it would work.  I don't think std.parallel is as strict as std.concurrency (and for pretty good reason).
> 
> I think a better way to mark progress is to make it an atomic integer type (like Artur has developed).

Yes. The problem with that however is that I never managed to make this do the right thing:

   Atomic!int a; // somewhere in a shared struct/class.
   ...
   int x = s.a;  // OK, access via getter.
   auto y = s.a; // Oops, we just copied the whole struct.
   void f(T)(T arg);
   f(s.a);       // Ditto.

Which may happen to work for properly aligned small structs because accessing those are atomic anyway, but is wrong.

artur
June 11, 2012
On 06/11/12 12:35, Steven Schveighoffer wrote:
> On Sun, 10 Jun 2012 10:22:50 -0400, Artur Skawina <art.08.09@gmail.com> wrote:
>> This is obviously fine:
>>
>>    const int[8] a; auto p = &a[0]; // typeof(p)==const(int)*
>>
>> as we just created the pointer, there's no need for it to be const, it just needs to point to 'const'.
>>
>> Now consider:
>>
>>    const int*[8] a; auto p = a[0]; // typeof(p)==const(int)*
>>
>> Here we *also* create a new pointer, so it does not need to be const either. The only difference is in how the *value* of this new pointer is obtained. In the 'const' case the data (pointer value) can just be read from the memory location and the operation is always perfectly safe.
> 
> The worst is this one:
> 
> const int[8] a;  auto e = a[0]; // typeof(p) == const(int) (in current compiler)

Yeah, and it makes using 'auto' less convenient - because then you have to cast away const.

>> The reason I don't want this to happen when dealing with 'shared' is not that it's wrong, it isn't. It's because it would make writing unsafe/confusing/buggy code too easy, as you then can completely ignore the 'shared' aspect.
> 
> I wholly disagree.  In fact, keeping the full qualifier intact *enforces* incorrect code, because you are forcing shared semantics on literally unshared data.
> 
> Never would this start ignoring shared on data that is truly shared.  This is why I don't really get your argument.
> 
> If you could perhaps explain with an example, it might be helpful.

*The programmer* can then treat shared data just like unshared. Because every
load and every store will "magically" work. I'm afraid that after more than
two or three people touch the code, the chances of it being correct would be
less than 50%...
The fact that you can not (or shouldn't be able to) mix shared and unshared
freely is one of the main advantages of shared-annotation.

artur
June 11, 2012
On Mon, 11 Jun 2012 07:51:37 -0400, Artur Skawina <art.08.09@gmail.com> wrote:

> On 06/11/12 12:26, Steven Schveighoffer wrote:
>> On Sat, 09 Jun 2012 08:31:01 -0400, Mike Wey <mike-wey@example.com> wrote:
>>
>>> On 06/09/2012 04:01 AM, mta`chrono wrote:
>>>> Would this be legal?
>>>>
>>>> class A
>>>> {
>>>>       private static shared int counter; // shared across all instances
>>>>
>>>>       this()
>>>>       {
>>>>            auto i = ++counter;
>>>>            pragma(msg, typeof(i)); // prints int
>>>>       }
>>>> }
>>>>
>>>
>>> Would it also be legal if the variable wasn't static?
>>
>> No.
>
> Why? What if this class would like to launch a few threads to do some work,
> export the address of the counter and have them report back by updating it?
>
> Unlike the shared-on-stack case, this wouldn't even be unsafe (the memory
> won't be freed until all threads stop using it.) The alternative is to have
> to split the class into two, more heap allocations etc.

The interesting thing here is, then you have both shared and unshared data in the same heap block.  Because a class is heap-allocated by default, you are right, you have a much smaller chance of sharing stack data.

However, allocating another heap block to do sharing, in my opinion, is worth the extra cost.  This way, you have clearly separated what is shared and what isn't.

You can always cast to get around the limitations.

auto sharedCounter = cast(shared int *)&counter;

dispatchThreadsToUpdateCounter(sharedCounter);

waitForThreadsToExit();

>> I think a better way to mark progress is to make it an atomic integer type (like Artur has developed).
>
> Yes. The problem with that however is that I never managed to make this
> do the right thing:
>
>    Atomic!int a; // somewhere in a shared struct/class.
>    ...
>    int x = s.a;  // OK, access via getter.
>    auto y = s.a; // Oops, we just copied the whole struct.
>    void f(T)(T arg);
>    f(s.a);       // Ditto.
>
> Which may happen to work for properly aligned small structs because
> accessing those are atomic anyway, but is wrong.

You can disable copying with @disable this(this);

-Steve
June 11, 2012
On Mon, 11 Jun 2012 07:56:12 -0400, Artur Skawina <art.08.09@gmail.com> wrote:

> On 06/11/12 12:35, Steven Schveighoffer wrote:

>> I wholly disagree.  In fact, keeping the full qualifier intact *enforces* incorrect code, because you are forcing shared semantics on literally unshared data.
>>
>> Never would this start ignoring shared on data that is truly shared.  This is why I don't really get your argument.
>>
>> If you could perhaps explain with an example, it might be helpful.
>
> *The programmer* can then treat shared data just like unshared. Because every
> load and every store will "magically" work. I'm afraid that after more than
> two or three people touch the code, the chances of it being correct would be
> less than 50%...
> The fact that you can not (or shouldn't be able to) mix shared and unshared
> freely is one of the main advantages of shared-annotation.

If shared variables aren't doing the right thing with loads and stores, then we should fix that.

But leaving things "the way they are" doesn't fix any problems:

shared int x;

void main()
{
    auto y = x; // typeof(y) == shared(int)
    y += 5;
    x = y;
}

How is this any more "correct" than if y is int?

-Steve
June 11, 2012
On 06/11/12 14:07, Steven Schveighoffer wrote:
> However, allocating another heap block to do sharing, in my opinion, is worth the extra cost.  This way, you have clearly separated what is shared and what isn't.
> 
> You can always cast to get around the limitations.

"clearly separating what is shared and what isn't" *is* exactly what tagging the data with 'shared' does.


>>> I think a better way to mark progress is to make it an atomic integer type (like Artur has developed).
>>
>> Yes. The problem with that however is that I never managed to make this do the right thing:
>>
>>    Atomic!int a; // somewhere in a shared struct/class.
>>    ...
>>    int x = s.a;  // OK, access via getter.
>>    auto y = s.a; // Oops, we just copied the whole struct.
>>    void f(T)(T arg);
>>    f(s.a);       // Ditto.
>>
>> Which may happen to work for properly aligned small structs because accessing those are atomic anyway, but is wrong.
> 
> You can disable copying with @disable this(this);

I wish.

   shared struct S { int x; @disable this(this); }
   shared S s;

Error: cannot implicitly convert expression (this) of type shared(S) to S

The post-dec/inc rewriting together with this bug also means you cannot prevent the bogus atomic++ operation from succeeding.

And that is not the only problem with 'shared' and structs.

http://www.digitalmars.com/d/archives/digitalmars/D/Disabling_copy_constructor_in_shared_structs_157638.html http://www.digitalmars.com/d/archives/digitalmars/D/dtors_in_shared_structs_fail_to_compile_157978.html

artur
June 11, 2012
On 06/11/12 14:11, Steven Schveighoffer wrote:
> On Mon, 11 Jun 2012 07:56:12 -0400, Artur Skawina <art.08.09@gmail.com> wrote:
> 
>> On 06/11/12 12:35, Steven Schveighoffer wrote:
> 
>>> I wholly disagree.  In fact, keeping the full qualifier intact *enforces* incorrect code, because you are forcing shared semantics on literally unshared data.
>>>
>>> Never would this start ignoring shared on data that is truly shared.  This is why I don't really get your argument.
>>>
>>> If you could perhaps explain with an example, it might be helpful.
>>
>> *The programmer* can then treat shared data just like unshared. Because every
>> load and every store will "magically" work. I'm afraid that after more than
>> two or three people touch the code, the chances of it being correct would be
>> less than 50%...
>> The fact that you can not (or shouldn't be able to) mix shared and unshared
>> freely is one of the main advantages of shared-annotation.
> 
> If shared variables aren't doing the right thing with loads and stores, then we should fix that.

Where do you draw the line?

shared struct S {
   int i
   void* p;
   SomeStruct s;
   ubyte[256] a;
}

shared(S)* p = ... ;

auto v1 = p.i;
auto v2 = p.p;
auto v3 = p.s;
auto v4 = p.a;
auto v5 = p.i++;

Are these operations on shared data all safe? Note that if these accesses would be protected by some lock, then the 'shared' qualifier wouldn't really be needed - compiler barriers, that make sure it all happens while this thread holds the lock, would be enough. (even the order of operations doesn't usually matter in that case and enforcing one would in fact add overhead)



> But leaving things "the way they are" doesn't fix any problems:
> 
> shared int x;
> 
> void main()
> {
>     auto y = x; // typeof(y) == shared(int)
>     y += 5;
>     x = y;
> }
> 
> How is this any more "correct" than if y is int?

Not allowing the implicit conversions does not fix all cases, yes.
It will catch /some/ invalid uses. It's a step towards a saner model.
Where "raw" access like in your example won't be allowed.
Yes, it would need a compiler flag.

When I say that I think your ideas are a step in the right direction, it's because I know where the journey will eventually lead us... :) I'm not sure forbidding direct manipulation is the ultimate goal, but it is a safe base for the next stage, which would be figuring out which cases can be allowed, because the compiler will always be able to do the right thing. Starting out by allowing everything is what got us into this mess in the first place.

artur
June 11, 2012
On Mon, 11 Jun 2012 09:39:40 -0400, Artur Skawina <art.08.09@gmail.com> wrote:

> On 06/11/12 14:07, Steven Schveighoffer wrote:
>> However, allocating another heap block to do sharing, in my opinion, is worth the extra cost.  This way, you have clearly separated what is shared and what isn't.
>>
>> You can always cast to get around the limitations.
>
> "clearly separating what is shared and what isn't" *is* exactly what
> tagging the data with 'shared' does.

There are special GC considerations for shared as well.  For instance, unshared data can go into a "local" heap.

But I feel uneasy passing around pointers to heap data to other threads where some of my thread-local data is present.  It's bound to lead to unwanted effects.

For example, if I share some piece of my class, and the other thread holds onto it forever, it means my class (which may hold large resources that aren't shared) will not be dealloced even when there's no reference to it outside that piece of shared data.

Also, don't forget, you can cast to get the behavior you desire.  You should always be able to work around these limitations with casting (and casting unshared to shared should be well-defined for the compiler as long as you don't ever treat the data as unshared again, similar to immutable).

I think it's reasonable to make it easier to write good designs, and harder to write questionable ones.  There's a lot of rules in D that are like that, just the whole notion of marking shared data is one of them.

I want to stress that this idea of preventing member variables from being marked shared is not necessarily a *requirement*, it's merely something I think fosters good design.  There's nothing technically wrong with it, I just think code is better off not doing it.

But marking stack variables as shared I think has to go -- there are too many pitfalls, a cast should be required.

>>>> I think a better way to mark progress is to make it an atomic integer type (like Artur has developed).
>>>
>>> Yes. The problem with that however is that I never managed to make this
>>> do the right thing:
>>>
>>>    Atomic!int a; // somewhere in a shared struct/class.
>>>    ...
>>>    int x = s.a;  // OK, access via getter.
>>>    auto y = s.a; // Oops, we just copied the whole struct.
>>>    void f(T)(T arg);
>>>    f(s.a);       // Ditto.
>>>
>>> Which may happen to work for properly aligned small structs because
>>> accessing those are atomic anyway, but is wrong.
>>
>> You can disable copying with @disable this(this);
>
> I wish.
>
>    shared struct S { int x; @disable this(this); }
>    shared S s;
> Error: cannot implicitly convert expression (this) of type shared(S) to S

This *definitely* is a bug.

>
> The post-dec/inc rewriting together with this bug also means you cannot
> prevent the bogus atomic++ operation from succeeding.
>
> And that is not the only problem with 'shared' and structs.
>
> http://www.digitalmars.com/d/archives/digitalmars/D/Disabling_copy_constructor_in_shared_structs_157638.html
> http://www.digitalmars.com/d/archives/digitalmars/D/dtors_in_shared_structs_fail_to_compile_157978.html

Haven't read these, but if there are bugs in the compiler, make sure you file those.  @disable this(this) *should* work, if it doesn't its a bug.

-Steve