p5p summary: Improving threads::shared ?

>  My sample worked OK *after* I removed the Data::Dumper
>  print; apparently, it has some issues w/ circular
>  references as well (I tried setting Deepcopy and
>  Purity, to no avail); I suspect its due to the fact
>  that references to shared elements don't have the same
>  stringified value, so it doesn't actually look like
>  a circular ref.

Oh, yes.  That's part of what I reported here:

http://rt.perl.org/rt3/Public/Bug/Display.html?id=37946

The following highlights the problem:

use strict;
use warnings;

use threads;
use threads::shared;

my $x;
$x = \$x;
share($x);

print("Look at \$x:\n");
print($x, "\n");
print($$x, "\n");
print($$$x, "\n");
print($$$$x, "\n");
print($$$$$x, "\n");
print($$$$$$x, "\n");

my @q :shared = ($x);

my $y = $q[0];

print("\nFirst look at \$y:\n");
print($y, "\n");
print($$y, "\n");
print($$$y, "\n");
print($$$$y, "\n");
print($$$$$y, "\n");
print($$$$$$y, "\n");

print("\nSecond look at \$y:\n");
print($y, "\n");
print($$y, "\n");
print($$$y, "\n");
print($$$$y, "\n");
print($$$$$y, "\n");
print($$$$$$y, "\n");

This outputs:

Look at $x:
REF(0x144f8f0)
REF(0x144f8f0)
REF(0x144f8f0)
REF(0x144f8f0)
REF(0x144f8f0)
REF(0x144f8f0)

First look at $y:
SCALAR(0x1423c70)
SCALAR(0x1423ad8)
SCALAR(0x14bd968)
SCALAR(0x14bd980)
SCALAR(0x14bd998)
SCALAR(0x14bd9b0)

Second look at $y:
REF(0x1423c70)
REF(0x1423ad8)
REF(0x14bd968)
REF(0x14bd980)
REF(0x14bd998)
SCALAR(0x14bd9b0)

Seems to me that this is a bug.  It shows that
threads::shared isn't detecting that it's dealing with a
circular reference.  With each level that the app traverses,
threads::shared "unrolls" another version of the shared
variable.

Jerry D. Hedden wrote:
>     Look at $x:
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>
>     First look at $y:
>     SCALAR(0x1423c70)
>     SCALAR(0x1423ad8)
>     SCALAR(0x14bd968)
>     SCALAR(0x14bd980)
>     SCALAR(0x14bd998)
>     SCALAR(0x14bd9b0)
>
>     Second look at $y:
>     REF(0x1423c70)
>     REF(0x1423ad8)
>     REF(0x14bd968)
>     REF(0x14bd980)
>     REF(0x14bd998)
>     SCALAR(0x14bd9b0)
>
>  Seems to me that this is a bug.  It shows that
>  threads::shared isn't detecting that it's dealing with a
>  circular reference.  With each level that the app traverses,
>  threads::shared "unrolls" another version of the shared
>  variable.

The more I think about it, the more I'm convinced this needs
to be fixed.  The original variable $x is a circular
reference, and it's finiteness is detectable.  The copy made
to $y has become an infinitely deep reference with lazy
evaluation.

Jerry D. Hedden wrote:
> erry D. Hedden wrote: 
> 
>
> I've been looking at how to fix this, but know understanding
> of the internals of threads::shared is weak.
>
> Conceptually, I think it would require keeping a weak
> reference of each private SV associated with a shared SV.
> This would need to be done on a per-thread basis.  Then when
> a thread tries to reference a shared SV, a lookup would be
> made to see if a private SV already exists (and still
> exists) for that thread.  If so, that is returned (and the
> ref count of the weak ref is incremented?).  If not, a new
> private SV is created, and a weak ref to it is appropriately
> stored.
>
> Is this a logical approach?  If so, is it doable?
>

By coincidence, I've been doing related
work on Thread::Sociable's STM implementation.
It has to keep a "shadow" proxy of each variable
in order to avoid the same referencing issue
(otherwise, it could create multiple different
transactional versions of the same referenced variable)

For threads::shared, the only solution I can think of is
adding a fieldhash to the thread-private my_ctx structure
keyed by the address of the referent variable's
shared version. Then each attempt to create a new proxy
would lookup any existing persistent proxy for the shared
SV, and return it if found (currently, each new reference
to a shared variable creates a new proxy, which is what causes
this mess).

It may create refcounting issues; the private proxy
would need to be refcounted every time the shared
variable was refcounted in the same thread. (Alternately,
the thread could refcount only the proxy, and then decrement
the shared versions refcount only when the private proxy's
refcount dropped to zero)

I also don't know how it would effect taking ref's of
shared array/hash elements.

And this will probably slow things down even further.

- Dean

Jerry D. Hedden wrote: 
>
> If circular references can't be fully supported in
> threads::shared, then I need to document this in its POD and
> in Thread::Queue's POD, too.  Do you agree?
>

I don't know that they can't be "supported"; but they do
need to be explained. I also don't see how it effects
T::Q: since you always skip over anything thats already
shared, existing circular refs aren't an issue. Its only
detecting and creating new shared circular refs that
causes a problem, and thats fixed w/ the fieldhash lookup
(which is keyed on the private address, not the shared
version).

For any existing apps (eg, Data::Dumper) that want to
deal with it, they could always fallback to detecting something
as shared and saving its id (ie, the shared interpretter version's
address) to detect cycles. Not pretty, but effective.

- Dean

Dean Arnold wrote:
> Jerry D. Hedden wrote: 
>
> For threads::shared, the only solution I can think of is
> adding a fieldhash to the thread-private my_ctx structure
> keyed by the address of the referent variable's
> shared version. Then each attempt to create a new proxy
> would lookup any existing persistent proxy for the shared
> SV, and return it if found (currently, each new reference
> to a shared variable creates a new proxy, which is what causes
> this mess).
>

I forgot one not-so-minor detail: clone processing
would need to be updated to
detect and replace the fieldhash'd proxies, and
update each SV that invoked the magic dup() method

- Dean

On Mon, May 5, 2008 at 1:53 PM, Jerry D. Hedden <jdhedden@cpan.org> wrote: 
>
>  Oh, yes.  That's part of what I reported here:
>
>     http://rt.perl.org/rt3/Public/Bug/Display.html?id=37946
>
>  The following highlights the problem:
>
>
>     use strict;
>     use warnings;
>
>     use threads;
>     use threads::shared;
>
>     my $x;
>     $x = \$x;
>     share($x);
>
>     print("Look at \$x:\n");
>     print($x, "\n");
>     print($$x, "\n");
>     print($$$x, "\n");
>     print($$$$x, "\n");
>     print($$$$$x, "\n");
>     print($$$$$$x, "\n");
>
>     my @q :shared = ($x);
>
>     my $y = $q[0];
>
>     print("\nFirst look at \$y:\n");
>     print($y, "\n");
>     print($$y, "\n");
>     print($$$y, "\n");
>     print($$$$y, "\n");
>     print($$$$$y, "\n");
>     print($$$$$$y, "\n");
>
>     print("\nSecond look at \$y:\n");
>     print($y, "\n");
>     print($$y, "\n");
>     print($$$y, "\n");
>     print($$$$y, "\n");
>     print($$$$$y, "\n");
>     print($$$$$$y, "\n");
>
>  This outputs:
>
>     Look at $x:
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>     REF(0x144f8f0)
>
>     First look at $y:
>     SCALAR(0x1423c70)
>     SCALAR(0x1423ad8)
>     SCALAR(0x14bd968)
>     SCALAR(0x14bd980)
>     SCALAR(0x14bd998)
>     SCALAR(0x14bd9b0)
>
>     Second look at $y:
>     REF(0x1423c70)
>     REF(0x1423ad8)
>     REF(0x14bd968)
>     REF(0x14bd980)
>     REF(0x14bd998)
>     SCALAR(0x14bd9b0)
>
>  Seems to me that this is a bug.  It shows that
>  threads::shared isn't detecting that it's dealing with a
>  circular reference.  With each level that the app traverses,
>  threads::shared "unrolls" another version of the shared
>  variable.

I just posted a patch to blead that fixes this.  If the patch passes
muster, I'll release an update for threads::shared to CPAN.

> > It means that something like this would DWIM: 
> ]; 
>
>  Would an assignment op overload work ?
>  I.e., if the LHS was already shared(), then the = overload
>  would do the deepcopy ? Or would that break the
>  XS tie/magic side of the code ?

Oops.  Shared variables aren't objects.  So I don't think we
can use 'overload' on '='.  Is that correct?

>  For any existing apps (eg, Data::Dumper) that want to
>  deal with it, they could always fallback to detecting
>  something as shared and saving its id (ie, the shared
>  interpretter version's address) to detect cycles. Not
>  pretty, but effective.

I looked into fixing Data::Dumper for this.  The circular
reference checking is done both in Perl (for the pure-Perl
version) and in XS (the usual version).  It'll take me
awhile to figure out how to do it in XS.

>  perhaps we could just provide a function:
>
>     $y = shared_clone($x);

What name should this function have?

shared_clone()
clone()
shared_copy()
copy()
make_shared()

Or something else?

Jerry D. Hedden wrote: 
>
> What name should this function have?
>
>     shared_clone()
>     clone()
>     shared_copy()
>     copy()
>     make_shared()
>
> Or something else?
>

I'd vote against clone() or copy(), as they're too
general. Otherwise, I personally have no opinion,
tho clone has come to mean this sort of deepcopy
(see the various Clone modules), so I suppose
shared_clone() may make sense.

- Dean