Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PUSH on tied arrays #10433

Closed
p5pRT opened this issue Jun 11, 2010 · 7 comments
Closed

PUSH on tied arrays #10433

p5pRT opened this issue Jun 11, 2010 · 7 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 11, 2010

Migrated from rt.perl.org#75676 (status was 'open')

Searchable as RT75676$

@p5pRT
Copy link
Author

p5pRT commented Jun 11, 2010

From @nwc10

Effectively this is 2 bugs in one ticket, but I can't find a practical way to
split them. Together, they mean that push on a shared array isn't atomic​:

$ cat push.pl
#!perl -w
use strict;
use threads;
use threads​::shared;

my @​array :shared;

sub hammer {
  for (0..9) {
  my $got = push @​array, 'L', 'R';
  print "Got got​: $got\n" if $got % 2;
  }
}

my @​t;
push @​t, threads->new(\&hammer) for 0..19;
$_->join foreach @​t;

print join('', @​array), "\n";

__END__
$ ./perl -Ilib push.pl
LRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLR
$ ./perl -Ilib push.pl
LRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLR
$ ./perl -Ilib push.pl
LRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLR
$ ./perl -Ilib push.pl
Got got​: 7
Got got​: 9
Got got​: 11
Got got​: 13
Got got​: 15
Got got​: 17
Got got​: 19
Got got​: 21
Got got​: 23
Got got​: 25
Got got​: 27
Got got​: 29
Got got​: 31
Got got​: 33
Got got​: 35
Got got​: 37
Got got​: 39
Got got​: 41
Got got​: 43
Got got​: 45
Got got​: 47
Got got​: 49
Got got​: 51
Got got​: 53
Got got​: 55
Got got​: 283
Got got​: 285
Got got​: 287
Got got​: 289
Got got​: 291
Got got​: 293
Got got​: 295
Got got​: 297
Got got​: 299
Got got​: 301
Got got​: 303
Got got​: 305
Got got​: 309
Got got​: 311
Got got​: 313
Got got​: 313
Got got​: 315
Got got​: 317
Got got​: 319
Got got​: 321
LRLRLLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLRLR

The return value of push is documented to be the number of elements in the
array. As every push is of two items, that means that after every push
completes the array will have an even number of entries.

Note that

1​: Sometimes the script gets an odd number returns from push
2​: Sometimes another push starts before the first completes

The problem seems to be twofold

1​: threads​::shared doesn't hold a lock for the duration of push. (It does for
  unshift.)

I can fix this. However, fixing it would hide the second bug​:

2​: For push and unshift, the PP code ignores the return value of PUSH and
  UNSHIFT, and makes a (second) call to the tied object, FETCHSIZE.

That feels wrong to me. I have a patch to fix that. (Which is how I stumbled
onto the threads​::shared inconsistency)

I believe that fixing all of this is the right thing to do, as currently the
implementation is inconsistent, less efficient than it could be, and prevents
some things from being done at all (such as atomic push and unshift semantics).

However, it does mean changing the exact behaviour of tied arrays from the
emergent behaviour they have had since the second day of their implementation​:

http​://perl5.git.perl.org/perl.git/commit/a60c0954410db87b # revision 424

[which amends the first attempt here
http​://perl5.git.perl.org/perl.git/commit/93965878572d85da # revision 418
]

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jun 12, 2010

From @iabyn

On Fri, Jun 11, 2010 at 06​:37​:56AM -0700, Nicholas Clark wrote​:

2​: For push and unshift, the PP code ignores the return value of PUSH and
UNSHIFT, and makes a (second) call to the tied object, FETCHSIZE.

Er, where does this happen? I can't see it, which means I'm probably
misinterpreting something.

--
This is a great day for France!
  -- Nixon at Charles De Gaulle's funeral

@p5pRT
Copy link
Author

p5pRT commented Jun 12, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Jun 12, 2010

From @nwc10

On Sat, Jun 12, 2010 at 12​:54​:05PM +0100, Dave Mitchell wrote​:

On Fri, Jun 11, 2010 at 06​:37​:56AM -0700, Nicholas Clark wrote​:

2​: For push and unshift, the PP code ignores the return value of PUSH and
UNSHIFT, and makes a (second) call to the tied object, FETCHSIZE.

Er, where does this happen? I can't see it, which means I'm probably
misinterpreting something.

No, it means that my description was inadequate. I'd forgotten that I'd also
had to do a bit of chasing to figure out *why* it wasn't completely buggy and
simply returning the size of the underlying array. This code at the end of
pp_push​:

  if (OP_GIMME(PL_op, 0) != G_VOID) {
  PUSHi( AvFILL(ary) + 1 );
  }

AvFILL expands to​:

#define AvFILL(av) ((SvRMAGICAL((const SV *) (av))) \
  ? mg_size(MUTABLE_SV(av)) : AvFILLp(av))

and for a tied array, SvRMAGICAL() is true, hence a call to mg_size() is made,
and that calls the FETCHSIZE method.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jun 12, 2010

From @iabyn

On Fri, Jun 11, 2010 at 06​:37​:56AM -0700, Nicholas Clark wrote​:

However, it does mean changing the exact behaviour of tied arrays from the
emergent behaviour they have had since the second day of their implementation​:

Hmm, looking at the various docs​:

Camel 2nd Ed says PUSH not done yet

Camel 3rd Ed (2000) doesn't say what PUSH should return, but its example
PUSH has push as its last statement, which means it returns the correct
value, but not explicitly.

perltie.pod, since 2000 (4ae8561) documents PUSH, but doesn't say what
it should return. Its example includes
  return $self->FETCHSIZE();
which is a hint that PUSH should return a value.

Tie​::Array's PUSH implementation looks to be explicitly wrong in its
return value, and perltie.pod recommends Tie​::Array as "a good starting
point for many tie() implementations".

My feeling is that the combination of ambiguous documentation, broken
Tie​::Array and 10-year old behaviour, means that it would be too risky
to change now.

--
Indomitable in retreat, invincible in advance, insufferable in victory
  -- Churchill on Montgomery

@toddr
Copy link
Member

toddr commented Feb 4, 2020

@iabyn should we close this then?

@toddr toddr added the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 4, 2020
@iabyn
Copy link
Contributor

iabyn commented Feb 5, 2020 via email

@toddr toddr removed the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 5, 2020
@toddr toddr closed this as completed Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants