On Sunday, 13 October 2013 at 07:47:55 UTC, Denis Shelomovskij wrote:
> --- Proposal ---
>
> The proposal is to add weak reference functionality based on `unstd.memory.weakref`. It can be placed e.g. in `core.memory`.
>
> Source code: https://bitbucket.org/denis-sh/unstandard/src/HEAD/unstd/memory/weakref.d
> Documentation: http://denis-sh.bitbucket.org/unstandard/unstd.memory.weakref.html
> Enhancement request: http://d.puremagic.com/issues/show_bug.cgi?id=4151
>
> --- Reasons ---
>
> Reasons, why we do need weak reference functionality in D (Issue 4151 [1]):
>
>   1. It has a general use.
>
>     I suppose no question here.
>
>
>   2. It's hard to implement correctly.
>
>     As a proof here are incorrect implementations written by experienced developers:
>       * Weak reference functionality in `std.signal` implementation:
>
> https://github.com/D-Programming-Language/phobos/blob/7134b603f8c9a2e9124247ff250c9b48ea697998/std/signals.d
>
>       * Alex's one from MCI:
>
> https://github.com/lycus/mci/blob/f9165c287f92e4ef70674828fbadb33ee3967547/src/mci/core/weak.d
>
>       * Robert's one from his new `std.signals` implementation proposal:
>
> https://github.com/phobos-x/phobosx/blob/d0cc6b45511465ef1d493b0d7226ccb990ae84e8/source/phobosx/signal.d
>
>       * My implementation (fixed now, I hope):
>
> https://bitbucket.org/denis-sh/unstandard/src/cb9a835a9ff5/unstd/memory/weakref.d
>
>     Everybody can check his knowledge of concurrent programming and D GC by trying to understand what exactly every implementation does and where are race conditions. It's recommended to read implementations in the order provided here going to the next one as soon as you see why previous one is incorrect. For now the only [probably] fixed implementation is mine so one can see spoiler here:
>
> https://bitbucket.org/denis-sh/unstandard/history-node/HEAD/unstd/memory/weakref.d
>       (the first and the most fixing (spoiling you joy to understand everything yourself) commit is 6f59b33)
>
>
>   3. It's hard to create a good API design for it.
>
>     No jokes. E.g. there are two different behaviours of .NET weak references and even more in Java library.
>
>
>   4. It is needed for correct signals implementation in D.
>
>     The lack of correct signals implementation is one of [major?] disadvantages of D.
>     Bug report: http://d.puremagic.com/issues/show_bug.cgi?id=9606

+1 having weakref in phobos

On Sunday, 13 October 2013 at 18:11:38 UTC, Andrei Alexandrescu wrote:
> On 10/13/13 11:07 AM, Michael wrote:
>> And line 61: what exactly mean a two !! in alive property?
>
> "Convert this to bool".
>
> Andrei

Thanks)

On Sunday, 13 October 2013 at 07:47:55 UTC, Denis Shelomovskij wrote:
> --- Proposal ---
>
> The proposal is to add weak reference functionality based on `unstd.memory.weakref`. It can be placed e.g. in `core.memory`.

+1

13.10.2013 21:36, Robert пишет:
>
>>         * Robert's one from his new `std.signals` implementation proposal:
>>
>> https://github.com/phobos-x/phobosx/blob/d0cc6b45511465ef1d493b0d7226ccb990ae84e8/source/phobosx/signal.d
>>
>
> Obviously I don't see it, otherwise I would have fixed it. Maybe you
> could elaborate a bit on your claim? Your implementation uses an
> entirely different technique for hiding the reference so a direct
> comparison is quite hard.

1. Have you read `gc.gc.fullcollect`, I mean a general function structure, not every line? If not, read it or you have no idea how collection performs.

2. I'm surprised you do think your implementation is correct as calling code twice (`foreach(i; 0..2)`) is an obvious hack to decrease a variety of undesired threads execution order (as it have to execute in this order twice).

----- Explanation -----

1. Race condition
  In every moment GC thread can be paused in state it already marked all dead blocks and ready to collect. So before `GC.addrOf` (which will have to wait for the end of the collection as it uses same mutex) call it can collect your object (the object can be on stack or whatever, doesn't matter). Also an new object can be created occupying the same memory and `GC.addrOf` will return non-null.

2. Incorrect assumption
  `o = GC.addrOf(tmp.address)` is just incorrect as you assume the object is placed at the base address of its memory block which is not guaranteed. Yes, it may be true for now (I haven't read GC sources enough to be definite here) in general case but what about some e.g. tricky user defined class instance sequences which user may create? Yes, never heard about it and just invented it, but it doesn't make this or similar case impossible. Also it's rather bad to do any needless assumption about internal stuff.

----- P.S. -----

I have to say you have a big problem especially for a programmer: you think you are competent in area you aren't and it can play a trick on you later. Please don't be angry with me, we all like to think so but we all have to look at ourselves as critically as possible to prevent problems in future.

-- 
Денис В. Шеломовский
Denis V. Shelomovskij

13.10.2013 22:19, Walter Bright пишет:
> On 10/13/2013 12:47 AM, Denis Shelomovskij wrote:
>> --- Proposal ---
>
> Please post as a DIP:
>
> http://wiki.dlang.org/DIPs
>
> The trouble with it as a n.g. posting is they tend to scroll off and be
> forgotten.

Why? There is already enhancement request 4151 to no forget and review queue to add it into.

-- 
Денис В. Шеломовский
Denis V. Shelomovskij

>
> 1. Have you read `gc.gc.fullcollect`, I mean a general function structure, not every line? If not, read it or you have no idea how collection performs.

I haven't, I relied on: http://dlang.org/garbage.html , but I
will now - thanks. If the information at garbage.html isn't
completely wrong I have some idea though.

>
> 2. I'm surprised you do think your implementation is correct as calling code twice (`foreach(i; 0..2)`) is an obvious hack to decrease a variety of undesired threads execution order (as it have to execute in this order twice).

Maybe you should read the code twice ;-) In the second run I
already have a visible address to the object. I retrieve it a
second time, because exactly the GC could have kicked in right
before seeing it, but in this case my invisible address gets
reset to null so I detect this case in the second iteration
(where it can't be collected any longer). The solution might not
be perfect and there might be a better one, but in my view it
should work.

> ----- Explanation -----
>
> 1. Race condition
>   In every moment GC thread can be paused in state it already marked all dead blocks and ready to collect. So before `GC.addrOf` (which will have to wait for the end of the collection as it uses same mutex) call it can collect your object (the object can be on stack or whatever, doesn't matter). Also an new object can be created occupying the same memory and `GC.addrOf` will return non-null.

Exactly the reason why I retrieve it twice. This way I detect a
collection and reassignment, because my invisible address would
have been set to null. With GC.addrof I ensure that it was
already set to null before checking.

>
> 2. Incorrect assumption
>   `o = GC.addrOf(tmp.address)` is just incorrect as you assume the object is placed at the base address of its memory block which is not guaranteed. Yes, it may be true for now (I haven't read GC sources enough to be definite here) in general case but what about some e.g. tricky user defined class instance sequences which user may create? Yes, never heard about it and just invented it, but it doesn't make this or similar case impossible. Also it's rather bad to do any needless assumption about internal stuff.

That's actually a good catch, thank you. I took it from another
implementation without proper checking. Will fix it.

>
> ----- P.S. -----
>
> I have to say you have a big problem especially for a programmer: you think you are competent in area you aren't and it can play a trick on you later. Please don't be angry with me, we all like to think so but we all have to look at ourselves as critically as possible to prevent problems in future.

Why would I be angry with a stranger who insults me in public? I
don't understand your concerns.

I did not write my signal implementation because of this bug, but
because of a number of other issues. I happened to stumble across
this one too, so I obviously had to fix it. If you are more
experienced in this area I am glad if you share your insights and
if you think something is wrong, I am glad to discuss it and fix
it if you are right, but just saying your implementation is
wrong, does not really help. It implies that you are obviously
right and everyone who does not see this is obviously a moron, if
someone has a bit of a problem with his ego, I don't think it is
me.

14.10.2013 13:04, robert пишет:
>
> Why would I be angry with a stranger who insults me in public? I
> don't understand your concerns.

No insults assumed! Just ugly truth about all of us. )

> If you are more
> experienced in this area I am glad if you share your insights and

Walter and Andrei often do silly mistakes. Can we suppose we are more experienced than they? Of course no, they just don't have enough time to think and check. Here the situation can be the same. I probably just have enough time to investigate the problem and solve it.

> if you think something is wrong, I am glad to discuss it and fix
> it if you are right, but just saying your implementation is
> wrong, does not really help. It implies that you are obviously
> right and everyone who does not see this is obviously a moron, if
> someone has a bit of a problem with his ego, I don't think it is
> me.

Easy, man. I have never met morons here, except, probably, myself.
Concurrent programming is fun so I just don't want to spoil the pleasure of investigation. And yes, I'm also lazy. )

Now about your code. First, I was completely incorrect about it, sorry for that. My mistake. I didn't even think the code containing such loop can be "so much correct". But, and this is the second, the code can't be "more or less" correct. It is either correct or not correct. It remembers me our (Russian) recent Moscow mayoral elections when we tried to change something in our country (we failed, yes) and after government won officials said: "It was the most honest elections of all preceding." )

So you code is incorrect and lets show it. When you give your code for eating to the compiler, it can does whatever it want but guarantee your program will work as you have written it (except special cases like copy construction elimination) and it doesn't assume every variable can be accessed from other threads. E.g. here is you code with unwinded loop in SSA (static single assignment) form:
```
auto tmp1 = atomicLoad(_obj);
void* o1 = tmp1.address;
if(o1 is null) return null;
void* o2 = GC.addrOf(tmp1.address);

auto tmp2 = atomicLoad(_obj);
void* o3 = tmp2.address;
if(o3 is null) return null;
void* o4 = GC.addrOf(tmp2.address);

if(o4) return cast(Object) o4;
return null;
```

`o1` is only used once and `o2` is never used so the compiler is free to discard the former and ignore the latter. So your code equals to this code:
```
auto tmp1 = atomicLoad(_obj);
if(tmp1.address is null) return null;
GC.addrOf(tmp1.address);

auto tmp2 = atomicLoad(_obj);
if(tmp2.address is null) return null;
void* o4 = GC.addrOf(tmp2.address);

if(o4) return cast(Object) o4;
return null;
```

So the second iteration gives nothing except decreasing (squaring to be precise) a variety of undesired threads execution order.

-- 
Денис В. Шеломовский
Denis V. Shelomovskij

>
> Easy, man. I have never met morons here, except, probably, myself.

My apologies if I got you wrong!

>
> So you code is incorrect and lets show it. When you give your code for eating to the compiler, it can does whatever it want but guarantee your program will work as you have written it (except special cases like copy construction elimination) and it doesn't assume every variable can be accessed from other threads. E.g. here is you code with unwinded loop in SSA (static single assignment) form:
> ...

Damn it, you are right I did not think this through, somehow thought the use in addrOf is enough, which is of course crap. Thank's a lot for your time, I'll fix this ASAP. It is so obviously wrong now, it really hurts. Ouch.

Thanks again!

14.10.2013 17:42, robert пишет:
> Damn it, you are right I did not think this through, somehow thought the
> use in addrOf is enough, which is of course crap. Thank's a lot for your
> time, I'll fix this ASAP.

So, here are your revised version:
https://github.com/phobos-x/phobosx/blob/1f0016c84c2043da0b9d2dafe65f54fcf6b6b8fa/source/phobosx/signal.d

Sorry, but you are making the same mistake again.

Lets start from the hardware. Just like a compiler CPU is free to do whatever it wants with passed instructions but guarantee result state will be the same as if it is executed sequentially. And it doesn't assume access from other threads by default (see e.g. "out-of-order execution"). So memory barriers (memory fences) are needed to ensure loads/stores before the barrier are performed and no loads/stores after the barrier are executing. This is what `core.atomic.atomicFence` does and it can be used in e.g. in mutex implementations. As your operations with `_obj` are already atomic no `atomicFence` call is needed.

Now let's assume without loss of generality `InvisibleAddress.address` returns `cast(void*) ~_addr`, inline the `address` call, and remove redundant `atomicFence` call:
```
auto tmp = atomicLoad(_obj);
auto o = cast(void*) ~tmp._addr;
if(o is null) return null;
GC.addrOf(o);

auto tmp1 = atomicLoad(_obj);
if(o is cast(void*) ~tmp1._addr)
    return cast(Object) o;
assert(cast(void*) ~tmp1._addr is null);
return null;
```

As I mentioned above you are making the same incorrect assumption that you know what machine instructions a compiler will generate. Never make such assumptions. Here is an example of how your code can be rewritten by a compiler:
```
auto tmp = atomicLoad(_obj);
if(tmp._addr == -1) return null;
GC.addrOf(cast(void*) ~tmp._addr);

auto tmp1 = atomicLoad(_obj);
if(tmp._addr == tmp1._addr)
    return cast(Object) cast(void*) ~tmp._addr;
assert(tmp1._addr == -1);
return null;
```


-- 
Денис В. Шеломовский
Denis V. Shelomovskij

Perhaps I missed it from skimming, but why are we using atomic operations here anyway?  Has testing revealed that it's necessary?