Smalltalk › Squeak › Squeak - Dev

#streamContents: on OrderedCollection broken?

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

8 messages Options

marcel.taeumel (old)

#streamContents: on OrderedCollection broken?

Should this work?

OrderedCollection streamContents: [:result |
result nextPutAll: #(a b c) asOrderedCollection].

Well, this works:

OrderedCollection streamContents: [:result |
result nextPutAll: #(a b c)].

And this works, too:

OrderedCollection streamContents: [:result |
result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].

Best,
Marcel

David T. Lewis

Re: #streamContents: on OrderedCollection broken?

On Mon, May 26, 2014 at 02:56:20AM -0700, Marcel Taeumel wrote:
> Should this work?
>
> OrderedCollection streamContents: [:result |
> result nextPutAll: #(a b c) asOrderedCollection].
>

It certainly looks like it should work.

Camillo Bruni addressed this case in Pharo by intoducing #streamSpecies, which
seems like a reasonable approach.

Discussion was here: http://forum.world.st/WriteStream-nextPutAll-OrderedCollections-td4634624.html

Dave

> Well, this works:
>
> OrderedCollection streamContents: [:result |
> result nextPutAll: #(a b c)].
>
> And this works, too:
>
> OrderedCollection streamContents: [:result |
> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].
>
> Best,
> Marcel
>

David T. Lewis

Re: #streamContents: on OrderedCollection broken?

On Mon, May 26, 2014 at 08:15:32AM -0400, David T. Lewis wrote:

> On Mon, May 26, 2014 at 02:56:20AM -0700, Marcel Taeumel wrote:
> > Should this work?
> >
> > OrderedCollection streamContents: [:result |
> > result nextPutAll: #(a b c) asOrderedCollection].
> >
>
> It certainly looks like it should work.
>
> Camillo Bruni addressed this case in Pharo by intoducing #streamSpecies, which
> seems like a reasonable approach.
>
> Discussion was here: http://forum.world.st/WriteStream-nextPutAll-OrderedCollections-td4634624.html
>

I tried the Pharo change in Squeak. It is a seemingly small change, but it breaks
WriteStreamTest>>testStreamContentsPositioning, which is a serious failure. I think
this means that an important performance optimization for String>>streamContents
would be broken. Pharo probably does not have that optimization, so the problem
would not be noticed there.

Dave

Levente Uzonyi-2

Re: #streamContents: on OrderedCollection broken?

In reply to this post by marcel.taeumel (old)

On Mon, 26 May 2014, Marcel Taeumel wrote:

> Should this work?
>
> OrderedCollection streamContents: [:result |
> result nextPutAll: #(a b c) asOrderedCollection].
>
> Well, this works:
>
> OrderedCollection streamContents: [:result |
> result nextPutAll: #(a b c)].
>
> And this works, too:
>
> OrderedCollection streamContents: [:result |
> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].

Even if it would work, it's a bad pattern, don't use it. OrderedCollection
implements its own streaming API (#add:, #addAll:), which is a lot more
efficient. I would use the same API as in streams in my own Smalltalk, but
that's another story.

Also, converting Arrays to OrderedCollections just to throw them away in
the next step is just as bad as creating a stream over an
OrderedCollection. Don't do it.

Levente

>
> Best,
> Marcel
>
>
>
> --
> View this message in context: http://forum.world.st/streamContents-on-OrderedCollection-broken-tp4760369.html
> Sent from the Squeak - Dev mailing list archive at Nabble.com.
>
>

Bert Freudenberg

Re: #streamContents: on OrderedCollection broken?

On 2014-05-26, at 18:40, Levente Uzonyi <[hidden email]> wrote:

> On Mon, 26 May 2014, Marcel Taeumel wrote:
>
>> Should this work?
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c) asOrderedCollection].
>>
>> Well, this works:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c)].
>>
>> And this works, too:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].
>
> Even if it would work, it's a bad pattern, don't use it. OrderedCollection implements its own streaming API (#add:, #addAll:), which is a lot more efficient. I would use the same API as in streams in my own Smalltalk, but that's another story.
>
> Also, converting Arrays to OrderedCollections just to throw them away in the next step is just as bad as creating a stream over an OrderedCollection. Don't do it.
>
>
> Levente

Still, it should work, don't you think?

- Bert -

smime.p7s (5K) Download Attachment

Nicolas Cellier

Re: #streamContents: on OrderedCollection broken?

2014-05-26 21:00 GMT+02:00 Bert Freudenberg <[hidden email]>:

On 2014-05-26, at 18:40, Levente Uzonyi <[hidden email]> wrote:

> On Mon, 26 May 2014, Marcel Taeumel wrote:
>
>> Should this work?
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c) asOrderedCollection].
>>
>> Well, this works:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c)].
>>
>> And this works, too:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].
>
> Even if it would work, it's a bad pattern, don't use it. OrderedCollection implements its own streaming API (#add:, #addAll:), which is a lot more efficient. I would use the same API as in streams in my own Smalltalk, but that's another story.
>
> Also, converting Arrays to OrderedCollections just to throw them away in the next step is just as bad as creating a stream over an OrderedCollection. Don't do it.
>
>
> Levente

Still, it should work, don't you think?

- Bert -

It would work if only the implementation of Stream was less bloated.

With current state of code, making it work might have a hard price for low reward IMO.

But I didn't look at exact details of the failure, maybe adding one more patch will do it... (until the next bug)

Nicolas Cellier

Re: #streamContents: on OrderedCollection broken?

2014-05-26 21:23 GMT+02:00 Nicolas Cellier <[hidden email]>:

2014-05-26 21:00 GMT+02:00 Bert Freudenberg <[hidden email]>:

On 2014-05-26, at 18:40, Levente Uzonyi <[hidden email]> wrote:

> On Mon, 26 May 2014, Marcel Taeumel wrote:
>
>> Should this work?
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c) asOrderedCollection].
>>
>> Well, this works:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPutAll: #(a b c)].
>>
>> And this works, too:
>>
>> OrderedCollection streamContents: [:result |
>> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].
>
> Even if it would work, it's a bad pattern, don't use it. OrderedCollection implements its own streaming API (#add:, #addAll:), which is a lot more efficient. I would use the same API as in streams in my own Smalltalk, but that's another story.
>
> Also, converting Arrays to OrderedCollections just to throw them away in the next step is just as bad as creating a stream over an OrderedCollection. Don't do it.
>
>
> Levente

Still, it should work, don't you think?

- Bert -

It would work if only the implementation of Stream was less bloated.
With current state of code, making it work might have a hard price for low reward IMO.

But I didn't look at exact details of the failure, maybe adding one more patch will do it... (until the next bug)

I see no simple hack at first glance

Note that this (Xtreams) one is working OK:

(OrderedCollection new: 3) writing write: #(a b c) asOrderedCollection; conclusion.

Levente Uzonyi-2

Re: #streamContents: on OrderedCollection broken?

In reply to this post by Bert Freudenberg

On Mon, 26 May 2014, Bert Freudenberg wrote:

>
> On 2014-05-26, at 18:40, Levente Uzonyi <[hidden email]> wrote:
>
>> On Mon, 26 May 2014, Marcel Taeumel wrote:
>>
>>> Should this work?
>>>
>>> OrderedCollection streamContents: [:result |
>>> result nextPutAll: #(a b c) asOrderedCollection].
>>>
>>> Well, this works:
>>>
>>> OrderedCollection streamContents: [:result |
>>> result nextPutAll: #(a b c)].
>>>
>>> And this works, too:
>>>
>>> OrderedCollection streamContents: [:result |
>>> result nextPut: #foo; nextPutAll: #(a b c) asOrderedCollection].
>>
>> Even if it would work, it's a bad pattern, don't use it. OrderedCollection implements its own streaming API (#add:, #addAll:), which is a lot more efficient. I would use the same API as in streams in my own Smalltalk, but that's another story.
>>
>> Also, converting Arrays to OrderedCollections just to throw them away in the next step is just as bad as creating a stream over an OrderedCollection. Don't do it.
>>
>>
>> Levente
>
> Still, it should work, don't you think?

Or just raise an error. Oh, wait, that's already the case, it just needs a
better error message. :)

The cause of the problem is that WriteStream assumes that the underlying
collection has the required number of slots exposed by #at:, #at:put: and
#size. This is not the case with OrderedCollection because it uses another
collection and hides its internals.

So if you just want to make it work - and let the bad practice spread -
then just replace #new: with #ofSize: when the collection is (re)created
in WriteStream (and superclasses if any).

For example changing #new: to #ofSize: in WriteStream >> #growTo: makes
the first snippet work.
The same can be done in SequenceableCollection >> #new:streamContents: to
decrease the number of unnecessary allocations by one.

But this fix won't work for other SequenceableCollections like LinkedList
or Heap (which shouldn't really be a SequenceableCollection anyway).
So I think the right thing to do is to implement #new:streamContents: in
these classes (including OrderedCollection) as

self shouldNotImplement

Levente

>
> - Bert -
>
>
>