Smalltalk › Squeak › Squeak VM

Bug in Interpreter>>primitiveNextPut:

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

11 messages Options

Henrik Sperre Johansen

Bug in Interpreter>>primitiveNextPut:

Interpreter >> primitiveNextPut:
limit := self fetchInteger: StreamReadLimitIndex ofObject: stream.
atIx := (array bitAnd: AtCacheMask) + AtPutBase.
(index < limit and: [(atCache at: atIx+AtCacheOop) = array])

so basically, it needs ReadLimit to be > 0. (Which is only true for (Read)WriteStreams if created with on:from:to:).

Changing the first line to
limit := self fetchInteger: StreamWriteLimitIndex ofObject: stream.

and the primitive now works.
ws := WriteStream on: (String new: 500000).
[1 to: 500000 do: [:ix | ws nextPut: $a]] timeToRun

before: 130
after: 33

Inserted a counter after the prim call, for the loop above it was always >= 500000, with fix it's >=1.

Cheers,
Henry

David T. Lewis

Re: Bug in Interpreter>>primitiveNextPut:

I opened a Mantis bug report for this issue:

http://bugs.squeak.org/view.php?id=7421

Dave

On Thu, Nov 26, 2009 at 03:10:52PM +0100, Henrik Johansen wrote:

>
> Interpreter >> primitiveNextPut:
> limit := self fetchInteger: StreamReadLimitIndex ofObject: stream.
> atIx := (array bitAnd: AtCacheMask) + AtPutBase.
> (index < limit and: [(atCache at: atIx+AtCacheOop) = array])
>
> so basically, it needs ReadLimit to be > 0. (Which is only true for (Read)WriteStreams if created with on:from:to:).
>
> Changing the first line to
> limit := self fetchInteger: StreamWriteLimitIndex ofObject: stream.
>
> and the primitive now works.
> ws := WriteStream on: (String new: 500000).
> [1 to: 500000 do: [:ix | ws nextPut: $a]] timeToRun
>
> before: 130
> after: 33
>
> Inserted a counter after the prim call, for the loop above it was always >= 500000, with fix it's >=1.
>
> Cheers,
> Henry
>

Eliot Miranda-2

Re: Bug in Interpreter>>primitiveNextPut:

On Thu, Nov 26, 2009 at 8:43 AM, David T. Lewis <[hidden email]> wrote:

I opened a Mantis bug report for this issue:

http://bugs.squeak.org/view.php?id=7421

Dave

I wouldn't bother trying to fix the primitive. Both in VisualWorks and at Teleplace the experience has been the same, getting rid of the primitives for next, nextPut: and atEnd sped things up. The system is simply too complex nowadays for these primitives to pay for themselves because they only cover a small number of cases. The primitives support Array and ByteString bow there are so many different kinds of arrays being streamed over that the cost of primitive failures outweigh the benefits of primitive successes.

On Thu, Nov 26, 2009 at 03:10:52PM +0100, Henrik Johansen wrote:
>
> Interpreter >> primitiveNextPut:
> limit := self fetchInteger: StreamReadLimitIndex ofObject: stream.
> atIx := (array bitAnd: AtCacheMask) + AtPutBase.
> (index < limit and: [(atCache at: atIx+AtCacheOop) = array])
>
> so basically, it needs ReadLimit to be > 0. (Which is only true for (Read)WriteStreams if created with on:from:to:).
>
> Changing the first line to
> limit := self fetchInteger: StreamWriteLimitIndex ofObject: stream.
>
> and the primitive now works.
> ws := WriteStream on: (String new: 500000).
> [1 to: 500000 do: [:ix | ws nextPut: $a]] timeToRun
>
> before: 130
> after: 33
>
> Inserted a counter after the prim call, for the loop above it was always >= 500000, with fix it's >=1.
>
> Cheers,
> Henry
>

Levente Uzonyi-2

Re: Bug in Interpreter>>primitiveNextPut:

On Thu, 26 Nov 2009, Eliot Miranda wrote:
>
> I wouldn't bother trying to fix the primitive. Both in VisualWorks and
> at Teleplace the experience has been the same, getting rid of the
> primitives for next, nextPut: and atEnd sped things up. The system is
> simply too complex nowadays for these primitives to pay for themselves
> because they only cover a small number of cases. The primitives support
> Array and ByteString bow there are so many different kinds of arrays being
> streamed over that the cost of primitive failures outweigh the benefits
> of primitive successes.

We are using these primitives in our own stream implementation like this:

next

<primitive: 65>
[ position < readLimit ] whileFalse: [ self receiveData ].
^buffer at: (position := position + 1)

And it does make a difference (buffer is a 8kiB sized ByteArray). Would
this be faster without the primitive?

Levente

Eliot Miranda-2

Re: Bug in Interpreter>>primitiveNextPut:

Hi Levente,

On Thu, Nov 26, 2009 at 12:01 PM, Levente Uzonyi <[hidden email]> wrote:

On Thu, 26 Nov 2009, Eliot Miranda wrote:

I wouldn't bother trying to fix the primitive. Both in VisualWorks and at Teleplace the experience has been the same, getting rid of the primitives for next, nextPut: and atEnd sped things up. The system is simply too complex nowadays for these primitives to pay for themselves because they only cover a small number of cases. The primitives support Array and ByteString bow there are so many different kinds of arrays being streamed over that the cost of primitive failures outweigh the benefits of primitive successes.

We are using these primitives in our own stream implementation like this:

next

<primitive: 65>
[ position < readLimit ] whileFalse: [ self receiveData ].
^buffer at: (position := position + 1)

And it does make a difference (buffer is a 8kiB sized ByteArray). Would this be faster without the primitive?

I can't guarantee it. You'd have to measure. All I can say is that we found it better to get rid of the stream primitives in VisualWorks and in the Cog JIT. Looking at the Squeak VM code I think the primitives will still win in the interpreter; after all they use the same machinery as at: and at:put:. But in a JIT they'll likely loose.

Since the Cog JIT isn't available yet I'm not really being helpful. I should think before I blurt. Apologies.

Eliot

Levente

Henrik Sperre Johansen

Re: Bug in Interpreter>>primitiveNextPut:

On 26.11.2009 23:26, Eliot Miranda wrote:

 
Hi Levente,

On Thu, Nov 26, 2009 at 12:01 PM, Levente Uzonyi <[hidden email]> wrote:

On Thu, 26 Nov 2009, Eliot Miranda wrote:

I wouldn't bother trying to fix the primitive. Both in VisualWorks and at Teleplace the experience has been the same, getting rid of the primitives for next, nextPut: and atEnd sped things up. The system is simply too complex nowadays for these primitives to pay for themselves because they only cover a small number of cases. The primitives support Array and ByteString bow there are so many different kinds of arrays being streamed over that the cost of primitive failures outweigh the benefits of primitive successes.

We are using these primitives in our own stream implementation like this:

next

<primitive: 65>
[ position < readLimit ] whileFalse: [ self receiveData ].
^buffer at: (position := position + 1)

And it does make a difference (buffer is a 8kiB sized ByteArray). Would this be faster without the primitive?

I can't guarantee it. You'd have to measure. All I can say is that we found it better to get rid of the stream primitives in VisualWorks and in the Cog JIT. Looking at the Squeak VM code I think the primitives will still win in the interpreter; after all they use the same machinery as at: and at:put:. But in a JIT they'll likely loose.

Since the Cog JIT isn't available yet I'm not really being helpful. I should think before I blurt. Apologies.

Eliot

Levente

FWIW, I'd take a JIT anyday over this bug getting fixed :)
On the other hand, it's a one-word fix which merely improves performace while we're waiting (unless someone decides to remove the primtive calls from the image altogether, then it merely becomes obsolete), so I hardly see the harm in applying it.

Cheers,
Henry

PS. Speaking of unneccessary overhead, what about that collection class == ByteString part for _every_ stream nextPut: , when ByteString at:put: will handle the case correctly anyways if collection is in fact a ByteString? Not to mention how it performs for ByteString currencly, seeing as the primitive now _does_ fail every time.

David T. Lewis

Re: Bug in Interpreter>>primitiveNextPut:

In reply to this post by Henrik Sperre Johansen

On Thu, Nov 26, 2009 at 03:10:52PM +0100, Henrik Johansen wrote:

Can anyone comment as to whether the above fix is correct? It looks right
to me, but it would be good if someone familiar with #primitiveNextPut
and the AtCache could review it.

I note that this primitive has not changed since 1998, so this would
rate as a really excellent bug catch!

http://bugs.squeak.org/view.php?id=7421

Thanks,
Dave

Eliot Miranda-2

Re: Bug in Interpreter>>primitiveNextPut:

On Fri, Nov 27, 2009 at 9:03 AM, David T. Lewis <[hidden email]> wrote:

On Thu, Nov 26, 2009 at 03:10:52PM +0100, Henrik Johansen wrote:
>

> Interpreter >> primitiveNextPut:
> limit := self fetchInteger: StreamReadLimitIndex ofObject: stream.
> atIx := (array bitAnd: AtCacheMask) + AtPutBase.
> (index < limit and: [(atCache at: atIx+AtCacheOop) = array])
>
> so basically, it needs ReadLimit to be > 0. (Which is only true for (Read)WriteStreams if created with on:from:to:).
>
> Changing the first line to
> limit := self fetchInteger: StreamWriteLimitIndex ofObject: stream.
>
> and the primitive now works.
> ws := WriteStream on: (String new: 500000).
> [1 to: 500000 do: [:ix | ws nextPut: $a]] timeToRun
>
> before: 130
> after: 33
>
> Inserted a counter after the prim call, for the loop above it was always >= 500000, with fix it's >=1.

Can anyone comment as to whether the above fix is correct? It looks right
to me, but it would be good if someone familiar with #primitiveNextPut
and the AtCache could review it.

The fix is correct. c.f. the body of WriteStream>>nextPut:.

I note that this primitive has not changed since 1998, so this would
rate as a really excellent bug catch!

http://bugs.squeak.org/view.php?id=7421

Thanks,
Dave

Igor Stasenko

Re: Bug in Interpreter>>primitiveNextPut:

In reply to this post by Eliot Miranda-2

2009/11/27 Eliot Miranda <[hidden email]>:

>
> Hi Levente,
>
> On Thu, Nov 26, 2009 at 12:01 PM, Levente Uzonyi <[hidden email]> wrote:
>>
>> On Thu, 26 Nov 2009, Eliot Miranda wrote:
>>>
>>> I wouldn't bother trying to fix the primitive. Both in VisualWorks and at Teleplace the experience has been the same, getting rid of the primitives for next, nextPut: and atEnd sped things up. The system is simply too complex nowadays for these primitives to pay for themselves because they only cover a small number of cases. The primitives support Array and ByteString bow there are so many different kinds of arrays being streamed over that the cost of primitive failures outweigh the benefits of primitive successes.
>>
>> We are using these primitives in our own stream implementation like this:
>>
>> next
>>
>> <primitive: 65>
>> [ position < readLimit ] whileFalse: [ self receiveData ].
>> ^buffer at: (position := position + 1)
>>
>> And it does make a difference (buffer is a 8kiB sized ByteArray). Would this be faster without the primitive?
>
> I can't guarantee it. You'd have to measure. All I can say is that we found it better to get rid of the stream primitives in VisualWorks and in the Cog JIT. Looking at the Squeak VM code I think the primitives will still win in the interpreter; after all they use the same machinery as at: and at:put:. But in a JIT they'll likely loose.
> Since the Cog JIT isn't available yet I'm not really being helpful. I should think before I blurt. Apologies.

Apologies accepted. ;)
Nevertheless, i think that reducing the set of VM responsibilities by
moving them to image side is a good tendency, because the less
classes/formats VM knows about , the less complex it is, and system is
more flexible.

> Eliot
>>
>> Levente
>
>
>

--
Best regards,
Igor Stasenko AKA sig.

David T. Lewis

Re: Bug in Interpreter>>primitiveNextPut:

In reply to this post by Eliot Miranda-2

On Fri, Nov 27, 2009 at 11:38:59AM -0800, Eliot Miranda wrote:

>
> On Fri, Nov 27, 2009 at 9:03 AM, David T. Lewis <[hidden email]> wrote:
> >
> > On Thu, Nov 26, 2009 at 03:10:52PM +0100, Henrik Johansen wrote:
> > >
> > > Interpreter >> primitiveNextPut:
> > > limit := self fetchInteger: StreamReadLimitIndex ofObject: stream.
> > > atIx := (array bitAnd: AtCacheMask) + AtPutBase.
> > > (index < limit and: [(atCache at: atIx+AtCacheOop) = array])
> > >
> > > so basically, it needs ReadLimit to be > 0. (Which is only true for
> > (Read)WriteStreams if created with on:from:to:).
> > >
> > > Changing the first line to
> > > limit := self fetchInteger: StreamWriteLimitIndex ofObject: stream.
> > >
> > > and the primitive now works.
> > > ws := WriteStream on: (String new: 500000).
> > > [1 to: 500000 do: [:ix | ws nextPut: $a]] timeToRun
> > >
> > > before: 130
> > > after: 33
> > >
> > > Inserted a counter after the prim call, for the loop above it was always
> > >= 500000, with fix it's >=1.
> >
> > Can anyone comment as to whether the above fix is correct? It looks right
> > to me, but it would be good if someone familiar with #primitiveNextPut
> > and the AtCache could review it.
> >
>
> The fix is correct. c.f. the body of WriteStream>>nextPut:.

Thanks Eliot. The fix is now in VMMaker-dtl.153 on SqueakSource.

Congratulations to Henrik Johansen for finding a bug of rare vintage.
This bug has gone undetected in the VM since 12/14/1998. Thanks Henry!

Dave

Levente Uzonyi-2

Re: Bug in Interpreter>>primitiveNextPut:

In reply to this post by Eliot Miranda-2

> On Thu, 26 Nov 2009, Eliot Miranda wrote:
>
>
> I can't guarantee it. You'd have to measure. All I can say is that we
> found it better to get rid of the stream primitives in VisualWorks and
in
> the Cog JIT. Looking at the Squeak VM code I think the primitives will
> still win in the interpreter; after all they use the same machinery as
at:
> and at:put:. But in a JIT they'll likely loose.

That sounds great.

> Since the Cog JIT isn't available yet I'm not really being helpful. I
> should think before I blurt. Apologies.

No problem, I hope I can measure the difference "real soon now". :)

Levente

>
> Eliot