From @nwc10

Created by @nwc10

The current implementation of :unique is most definitely not threadsafe.
Which is, er, crazy, given that it's for use with ithreads. Aside from
all the second order bugs (threads can quit an clean up typeglobs that other
threads are pointing to, putting things of the wrong type into GvHV, GvAV
etc) it has some pretty big first order race conditions.

Setting SvREADONY() on a perl scalar *does not* stop the core modifying that
scalar. Notably you can

1​: still take a reference to a :unique object, which means modifying its
  reference count, which means that two threads are racing on this. In
  theory getting the race wrong on releasing a reference could drop the
  count to zero and cause the scalar to be freed while actually still in use.
  (but I can't win that race with my test case)

2​: still stringify a :unique object. (And for that matter promote its type,
  which moves the body)

3​: modify the elements of an array marked readonly

Modifying the existing gv_share code to recurse over anything it points to
is not going to solve the first two. The current implementation has to go.
Correctness is more important that speed. Or SEGVs.

Patches welcome for a better, correct, implementation. I think something
akin to threads​::shared would work, possibly with thin SVs/AVs/HVs that
each hold a reference on a shared object that actually has the read-only
data. (This way the shared object can't go away until all the owners have,
and the genuine per-thread front end objects mean that the core isn't
scribbling on a single SV from more than one thread)

Nicholas Clark

1​:

#!perl -w
use strict;

use threads;

package Foo;

our $bar = "Bug";

package main;

my $count = shift || 1000;
my @​threads;

foreach (0..15) {
  push @​threads, threads->create(
  sub {
  while ($count--) {
  my $r = \$Foo​::bar;
# print ref $r, ' ', Internals​::SvREFCNT($$r), "\n";
  }
# print ' ', Internals​::SvREFCNT($Foo​::bar), "\n";
  }
  );
}

$_->join foreach @​threads;
__END__

2​:

#!perl -w
use strict;

use threads;

package Foo;

our $bar : unique = -1.3e-5;

package main;

my $count = shift || 16;
my @​threads;

use Devel​::Peek;
Dump $Foo​::bar;

foreach (1..$count) {
  push @​threads, threads->create(
  sub {
  my $a = sprintf "%s", $Foo​::bar;
  }
  );
}

$_->join foreach @​threads;

Dump $Foo​::bar;
__END__

3​:

#!perl -w
use strict;

use threads;

package Foo;

our @​bar : unique = "aaa";

package main;

my $count = shift || 1000;
my @​threads;

foreach (0..15) {
  push @​threads, threads->create(
  sub {
  while ($count--) {
  ++$Foo​::bar[0];
  }
  }
  );
}

$_->join foreach @​threads;

print "$Foo​::bar[0]\n";
__END__

Flags:
    category=core
    severity=high

Site configuration information for perl vv5.9.3:

Configured by nick at Thu Jun 23 16:36:33 BST 2005.

Summary of my perl5 (revision 5 version 9 subversion 3 patch 24148) configuration:
  Platform:
    osname=linux, osvers=2.4.21-15.0.3.elsmp, archname=i686-linux-thread-multi
    uname='linux switch 2.4.21-15.0.3.elsmp #1 smp wed jul 7 09:34:05 edt 2004 i686 i686 i386 gnulinux '
    config_args='-Dusedevel=y -Dcc=ccache gcc -Dld=gcc -Ubincompat5005 -Uinstallusrbinperl -Dcf_email=nick@ccl4.org -Dperladmin=nick@ccl4.org -Dinc_version_list=  -Dinc_version_list_init=0 -Doptimize=-O2 -Dusethreads -Uuse64bitint -Duseperlio -Dprefix=~/Sandpit/snap5.9.x-24959 -Dinstallman1dir=none -Dinstallman3dir=none -de'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='ccache gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include'
    ccversion='', gccversion='3.2.3 20030502 (Red Hat Linux 3.2.3-49)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl vv5.9.3:
    lib
    /home/nick/Sandpit/snap5.9.x-24959/lib/perl5/5.9.3/i686-linux-thread-multi
    /home/nick/Sandpit/snap5.9.x-24959/lib/perl5/5.9.3
    /home/nick/Sandpit/snap5.9.x-24959/lib/perl5/site_perl/5.9.3/i686-linux-thread-multi
    /home/nick/Sandpit/snap5.9.x-24959/lib/perl5/site_perl/5.9.3
    /home/nick/Sandpit/snap5.9.x-24959/lib/perl5/site_perl
    .


Environment for perl vv5.9.3:
    HOME=/home/nick
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/nick/bin:/usr/kerberos/bin:/usr/lib/ccache/bin:/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin:/usr/local/sbin:/sbin:/usr/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @lizmat

At 4​:33 PM +0000 6/23/05, Nicholas Clark (via RT) wrote​:

# New Ticket Created by Nicholas Clark
# Please include the string​: [perl #36375]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=36375 >

This is a bug report for perl from nick@​ccl4.org,
generated with the help of perlbug 1.35 running under perl vv5.9.3.

-----------------------------------------------------------------
[Please enter your report here]

The current implementation of :unique is most definitely not threadsafe.
Which is, er, crazy, given that it's for use with ithreads. Aside from
all the second order bugs (threads can quit an clean up typeglobs that other
threads are pointing to, putting things of the wrong type into GvHV, GvAV
etc) it has some pretty big first order race conditions.

FWIW​: I think this nails the coffin on :unique.

I think the safest would be to deprecate "​:unique" by making it a
noop. And be done with it.

With the current state of parrot / pugs / perl 6 / ponie I don't
think it is worth the tuits to get "​:unique" right.

Just my 1.5 eurocents.

Liz

The RT System itself - Status changed from 'new' to 'open'

From @rgarcia

On 6/23/05, Elizabeth Mattijsen <liz@​dijkmat.nl> wrote​:

FWIW​: I think this nails the coffin on :unique.

I think the safest would be to deprecate "​:unique" by making it a
noop. And be done with it.

Strictly speaking, deprecation and removal are not the same thing.

I don't think there's much advanced threaded perl code out there, but
you never know. After all, a large number of systems ship a threaded
perl by default.

With the current state of parrot / pugs / perl 6 / ponie I don't
think it is worth the tuits to get "​:unique" right.

Except if it helps ponie.

From @lizmat

At 10​:54 PM +0200 6/23/05, Rafael Garcia-Suarez wrote​:

On 6/23/05, Elizabeth Mattijsen <liz@​dijkmat.nl> wrote​:

FWIW​: I think this nails the coffin on :unique.
I think the safest would be to deprecate "​:unique" by making it a
noop. And be done with it.
Strictly speaking, deprecation and removal are not the same thing.

You're right​: I meant "​:unique" to become a noop in blead (and 5.10
later), and to have it marked "deprecated" in 5.10.

I don't think there's much advanced threaded perl code out there, but
you never know. After all, a large number of systems ship a threaded
perl by default.

I think I've introduced a few "​:unique"'s in the Perl core modules in
my attempts to reduce the memory footprint of perl. I think the
winnings were very small indeed, especially if you compare it with
all the other work that has been done recently about reducing memory
footprint (Consting and folding of various structures).

With the current state of parrot / pugs / perl 6 / ponie I don't
think it is worth the tuits to get "​:unique" right.
Except if it helps ponie.

That was something I hadn't considered. Would it?

Liz

From @nwc10

On Thu, Jun 23, 2005 at 11​:24​:10PM +0200, Elizabeth Mattijsen wrote​:

At 10​:54 PM +0200 6/23/05, Rafael Garcia-Suarez wrote​:

On 6/23/05, Elizabeth Mattijsen <liz@​dijkmat.nl> wrote​:

FWIW​: I think this nails the coffin on :unique.
I think the safest would be to deprecate "​:unique" by making it a
noop. And be done with it.
Strictly speaking, deprecation and removal are not the same thing.

You're right​: I meant "​:unique" to become a noop in blead (and 5.10
later), and to have it marked "deprecated" in 5.10.

I see no actual harm in making it a no-op everywhere. (For now, pending
a viable implementation)
Or at least a compile time option.

I think I've introduced a few "​:unique"'s in the Perl core modules in
my attempts to reduce the memory footprint of perl. I think the
winnings were very small indeed, especially if you compare it with
all the other work that has been done recently about reducing memory
footprint (Consting and folding of various structures).

That can't go into maint.

However, I think I can see a reasonable efficient way of making unique
value scalars (specifically non-references). References, arrays and hashes
I'm not so sure about.

If a (unique) scalar in one thread is a reference back to a copy in a parent
thread, how does the child stop the parent thread going away. (or at least,
cause it not to globally destruct until the referring child destructs)
?

With the current state of parrot / pugs / perl 6 / ponie I don't
think it is worth the tuits to get "​:unique" right.
Except if it helps ponie.

That was something I hadn't considered. Would it?

I confess that taking out the current implementation would, because it
conflicts with removing the symbol table <=> typeglob reference loop.
(Which is causing big pain with global destruction when you try to do
that properly, which ponie is)

Nicholas Clark

From @rgarcia

On 6/23/05, Nicholas Clark <nick@​ccl4.org> wrote​:

I see no actual harm in making it a no-op everywhere. (For now, pending
a viable implementation)
Or at least a compile time option.

Your call :)

I think I've introduced a few "​:unique"'s in the Perl core modules in
my attempts to reduce the memory footprint of perl. I think the
winnings were very small indeed, especially if you compare it with
all the other work that has been done recently about reducing memory
footprint (Consting and folding of various structures).

I can't detect any :unique in blead modules.

With the current state of parrot / pugs / perl 6 / ponie I don't
think it is worth the tuits to get "​:unique" right.
Except if it helps ponie.

That was something I hadn't considered. Would it?

I confess that taking out the current implementation would, because it
conflicts with removing the symbol table <=> typeglob reference loop.
(Which is causing big pain with global destruction when you try to do
that properly, which ponie is)

So that's a big plus for removing it (which is already done in fact).
If it doesn't get replaced, we should deprecate :unique and have it
emit a warning.

From @nwc10

On Thu, Jun 23, 2005 at 11​:49​:14PM +0200, Rafael Garcia-Suarez wrote​:

I can't detect any :unique in blead modules.

The only one I know of is our $summary : unique in Config_heavy.pl

So that's a big plus for removing it (which is already done in fact).
If it doesn't get replaced, we should deprecate :unique and have it
emit a warning.

I actually like the idea of :unique. It's just finding a viable way to
implement it may prove more fun.

Then again, if copy on write can be made to work across threads, I guess it
all becomes irrelevant, as with that anything that is not modified is stored
only once.

Nicholas Clark

From @lizmat

At 10​:58 PM +0100 6/23/05, Nicholas Clark wrote​:

On Thu, Jun 23, 2005 at 11​:49​:14PM +0200, Rafael Garcia-Suarez wrote​:

I can't detect any :unique in blead modules.

The only one I know of is our $summary : unique in Config_heavy.pl

So that's a big plus for removing it (which is already done in fact).
If it doesn't get replaced, we should deprecate :unique and have it
emit a warning.

I actually like the idea of :unique. It's just finding a viable way to
implement it may prove more fun.

Then again, if copy on write can be made to work across threads, I guess it
all becomes irrelevant, as with that anything that is not modified is stored
only once.

Indeed. And you won't have to break your mind anymore where to add
"​:unique" and where not, as it will be all automagical then.

Liz

From @rgs

Nicholas Clark wrote​:

On Thu, Jun 23, 2005 at 11​:49​:14PM +0200, Rafael Garcia-Suarez wrote​:

I can't detect any :unique in blead modules.

The only one I know of is our $summary : unique in Config_heavy.pl

Removed as change #25195 in blead.

So that's a big plus for removing it (which is already done in fact).
If it doesn't get replaced, we should deprecate :unique and have it
emit a warning.

I actually like the idea of :unique. It's just finding a viable way to
implement it may prove more fun.

Then again, if copy on write can be made to work across threads, I guess it
all becomes irrelevant, as with that anything that is not modified is stored
only once.

OK. But until it's rewritten, I think we should turn it into a no-op that
emits a deprecation warning. Opinions ?

From @lizmat

At 1​:10 PM +0200 7/20/05, Rafael Garcia-Suarez wrote​:

Nicholas Clark wrote​:

On Thu, Jun 23, 2005 at 11​:49​:14PM +0200, Rafael Garcia-Suarez wrote​:

I can't detect any :unique in blead modules.

The only one I know of is our $summary : unique in Config_heavy.pl

Removed as change #25195 in blead.

So that's a big plus for removing it (which is already done in fact).
If it doesn't get replaced, we should deprecate :unique and have it
emit a warning.

I actually like the idea of :unique. It's just finding a viable way to
implement it may prove more fun.

Then again, if copy on write can be made to work across threads, I guess it
all becomes irrelevant, as with that anything that is not modified is stored
only once.

OK. But until it's rewritten, I think we should turn it into a no-op that
emits a deprecation warning. Opinions ?

Agree.

Liz

@cpansprout - Status changed from 'open' to 'resolved'