The Inbox: CollectionsTests-dtl.212.mcz
Locked
 
|
David T. Lewis uploaded a new version of CollectionsTests to project The Inbox:
http://source.squeak.org/inbox/CollectionsTests-dtl.212.mcz ==================== Summary ==================== Name: CollectionsTests-dtl.212 Author: dtl Time: 19 January 2014, 10:43:43.125 am UUID: 6916414f-dda9-4b64-a4ee-f5f50aacd4c4 Ancestors: CollectionsTests-dtl.211 Add OrderedCollectionTest>>testStreamContentsPositioning. String class>>new:streamContents: optimizes performance by answering originalCollection in the case of a stream positioned to the size of its original collection. This fails if the stream has been repositioned backwards to its original length. The optimization was introduced in Collections-ar.129 which merges Collections-ul.128 from inbox: - introduced #new:streamContents: in SequenceableCollection class. It's like #streamContents: but if you know the size of the new collection this method doesn't copy the result. - updated SequenceableCollection class >> #streamContents: to use #new:streamContents:, kept the original default size 100. =============== Diff against CollectionsTests-dtl.211 =============== Item was added: + ----- Method: OrderedCollectionTest>>testStreamContentsPositioning (in category 'testStreaming') ----- + testStreamContentsPositioning + "String class>>new:streamContents: optimizes performance by answering the + originalCollection in the case of a stream positioned to the size of the original + collection. This fails if the stream has been repositioned backwards to its original + length." + + "(OrderedCollectionTest selector: #testStreamContentsPositioning) debug" + + | s | + s := String new: 10 streamContents: [ :strm | + strm nextPutAll: 'XXXXX'. + self assert: 'XXXXX' equals: strm contents. + strm nextPut: $X. + self assert: 'XXXXXX' equals: strm contents. + strm position: strm position - 1. + self assert: 'XXXXX' equals: strm contents. + strm nextPutAll: 'XXXXX'. + self assert: 'XXXXXXXXXX' equals: strm contents. + strm nextPut: $X. + self assert: 'XXXXXXXXXXX' equals: strm contents. + strm position: strm position - 1. + self assert: 'XXXXXXXXXX' equals: strm contents. + ]. + self assert: 10 equals: s size. + self assert: 'XXXXXXXXXX' equals: s. + + ! |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
This goes in the inbox because I am not sure if it should be treated
as a bug or as an acceptable limitation that needs to be documented. Dave On Sun, Jan 19, 2014 at 03:43:45PM +0000, [hidden email] wrote: > David T. Lewis uploaded a new version of CollectionsTests to project The Inbox: > http://source.squeak.org/inbox/CollectionsTests-dtl.212.mcz > > ==================== Summary ==================== > > Name: CollectionsTests-dtl.212 > Author: dtl > Time: 19 January 2014, 10:43:43.125 am > UUID: 6916414f-dda9-4b64-a4ee-f5f50aacd4c4 > Ancestors: CollectionsTests-dtl.211 > > Add OrderedCollectionTest>>testStreamContentsPositioning. > > String class>>new:streamContents: optimizes performance by answering originalCollection in the case of a stream positioned to the size of its original collection. This fails if the stream has been repositioned backwards to its original length. > > The optimization was introduced in Collections-ar.129 which merges Collections-ul.128 from inbox: > > - introduced #new:streamContents: in SequenceableCollection class. It's like #streamContents: but if you know the size of the new collection this method doesn't copy the result. > - updated SequenceableCollection class >> #streamContents: to use #new:streamContents:, kept the original default size 100. > > =============== Diff against CollectionsTests-dtl.211 =============== > > Item was added: > + ----- Method: OrderedCollectionTest>>testStreamContentsPositioning (in category 'testStreaming') ----- > + testStreamContentsPositioning > + "String class>>new:streamContents: optimizes performance by answering the > + originalCollection in the case of a stream positioned to the size of the original > + collection. This fails if the stream has been repositioned backwards to its original > + length." > + > + "(OrderedCollectionTest selector: #testStreamContentsPositioning) debug" > + > + | s | > + s := String new: 10 streamContents: [ :strm | > + strm nextPutAll: 'XXXXX'. > + self assert: 'XXXXX' equals: strm contents. > + strm nextPut: $X. > + self assert: 'XXXXXX' equals: strm contents. > + strm position: strm position - 1. > + self assert: 'XXXXX' equals: strm contents. > + strm nextPutAll: 'XXXXX'. > + self assert: 'XXXXXXXXXX' equals: strm contents. > + strm nextPut: $X. > + self assert: 'XXXXXXXXXXX' equals: strm contents. > + strm position: strm position - 1. > + self assert: 'XXXXXXXXXX' equals: strm contents. > + ]. > + self assert: 10 equals: s size. > + self assert: 'XXXXXXXXXX' equals: s. > + > + ! > |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
It's a bug. Let's remove the optimizatoin. If we really want it, this could be a implemented like WriteStream>>sharedContents | sz | ^(sz := position max: readLimit) = collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 to: sz]2014/1/19 David T. Lewis <[hidden email]> This goes in the inbox because I am not sure if it should be treated |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
On Sun, 19 Jan 2014, Nicolas Cellier wrote:
> It's a bug. Let's remove the optimizatoin. If we really want it, this could be a implemented like > > WriteStream>>sharedContents | sz | ^(sz := position max: readLimit) = collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 to: > sz] #new:streamContents: is about optimization, nothing else. I think it's a better solution to set the stream to the end before comparing the position with the size, because I don't see any other possible use for #sharedContents. Levente P.S.: Why is this an OrderedCollectionTest? It's not related to OrderedCollection at all. > > > 2014/1/19 David T. Lewis <[hidden email]> > This goes in the inbox because I am not sure if it should be treated > as a bug or as an acceptable limitation that needs to be documented. > > Dave > > > On Sun, Jan 19, 2014 at 03:43:45PM +0000, [hidden email] wrote: > > David T. Lewis uploaded a new version of CollectionsTests to project The Inbox: > > http://source.squeak.org/inbox/CollectionsTests-dtl.212.mcz > > > > ==================== Summary ==================== > > > > Name: CollectionsTests-dtl.212 > > Author: dtl > > Time: 19 January 2014, 10:43:43.125 am > > UUID: 6916414f-dda9-4b64-a4ee-f5f50aacd4c4 > > Ancestors: CollectionsTests-dtl.211 > > > > Add OrderedCollectionTest>>testStreamContentsPositioning. > > > > String class>>new:streamContents: optimizes performance by answering originalCollection in the case of a stream positioned to the > size of its original collection. This fails if the stream has been repositioned backwards to its original length. > > > > The optimization was introduced in Collections-ar.129 which merges Collections-ul.128 from inbox: > > > > - introduced #new:streamContents: in SequenceableCollection class. It's like #streamContents: but if you know the size of the new > collection this method doesn't copy the result. > > - updated SequenceableCollection class >> #streamContents: to use #new:streamContents:, kept the original default size 100. > > > > =============== Diff against CollectionsTests-dtl.211 =============== > > > > Item was added: > > + ----- Method: OrderedCollectionTest>>testStreamContentsPositioning (in category 'testStreaming') ----- > > + testStreamContentsPositioning > > + "String class>>new:streamContents: optimizes performance by answering the > > + originalCollection in the case of a stream positioned to the size of the original > > + collection. This fails if the stream has been repositioned backwards to its original > > + length." > > + > > + "(OrderedCollectionTest selector: #testStreamContentsPositioning) debug" > > + > > + | s | > > + s := String new: 10 streamContents: [ :strm | > > + strm nextPutAll: 'XXXXX'. > > + self assert: 'XXXXX' equals: strm contents. > > + strm nextPut: $X. > > + self assert: 'XXXXXX' equals: strm contents. > > + strm position: strm position - 1. > > + self assert: 'XXXXX' equals: strm contents. > > + strm nextPutAll: 'XXXXX'. > > + self assert: 'XXXXXXXXXX' equals: strm contents. > > + strm nextPut: $X. > > + self assert: 'XXXXXXXXXXX' equals: strm contents. > > + strm position: strm position - 1. > > + self assert: 'XXXXXXXXXX' equals: strm contents. > > + ]. > > + self assert: 10 equals: s size. > > + self assert: 'XXXXXXXXXX' equals: s. > > + > > + ! > > > > > > |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
On Sun, Jan 19, 2014 at 06:06:37PM +0100, Levente Uzonyi wrote:
> On Sun, 19 Jan 2014, Nicolas Cellier wrote: > > P.S.: Why is this an OrderedCollectionTest? It's not related to > OrderedCollection at all. > You are right, it belongs somewhere else. I had found the existing #testStreamContents in OrderedCollection, so I just added another test there without thinking about it. WriteStreamTest looks like a better place to put it. Dave |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
In reply to this post by Levente Uzonyi-2
On Sun, Jan 19, 2014 at 06:06:37PM +0100, Levente Uzonyi wrote:
> On Sun, 19 Jan 2014, Nicolas Cellier wrote: > > >It's a bug. Let's remove the optimizatoin. If we really want it, this > >could be a implemented like > > > >WriteStream>>sharedContents? | sz | ^(sz := position max: readLimit) = > >collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 to: > >sz] > > #new:streamContents: is about optimization, nothing else. I think it's a > better solution to set the stream to the end before comparing the position > with the size, because I don't see any other possible use for > #sharedContents. Do you mean something like this? new: newSize streamContents: blockWithArg | stream p endPos | stream := WriteStream on: (self new: newSize). blockWithArg value: stream. p := stream position. endPos := stream setToEnd position. p = endPos ifTrue: [p = newSize ifTrue: [^ stream originalContents]] ifFalse: [stream position: p]. ^ stream contents I'm not sure what that would do to the performance gains. Dave |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
On Sun, 19 Jan 2014, David T. Lewis wrote: > On Sun, Jan 19, 2014 at 06:06:37PM +0100, Levente Uzonyi wrote: >> On Sun, 19 Jan 2014, Nicolas Cellier wrote: >> >>> It's a bug. Let's remove the optimizatoin. If we really want it, this >>> could be a implemented like >>> >>> WriteStream>>sharedContents? | sz | ^(sz := position max: readLimit) = >>> collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 to: >>> sz] >> >> #new:streamContents: is about optimization, nothing else. I think it's a >> better solution to set the stream to the end before comparing the position >> with the size, because I don't see any other possible use for >> #sharedContents. > > Do you mean something like this? > > new: newSize streamContents: blockWithArg > | stream p endPos | > stream := WriteStream on: (self new: newSize). > blockWithArg value: stream. > p := stream position. It doesn't matter what the current position of the stream is. If there's data after the current position, then #contents will return it. So I think this is how the method should be: new: newSize streamContents: blockWithArg | stream | stream := WriteStream on: (self new: newSize). blockWithArg value: stream. stream setToEnd position = newSize ifTrue: [ ^stream originalContents ] ifFalse: [ ^stream contents ] Levente > endPos := stream setToEnd position. > p = endPos > ifTrue: [p = newSize > ifTrue: [^ stream originalContents]] > ifFalse: [stream position: p]. > ^ stream contents > > > I'm not sure what that would do to the performance gains. > > Dave > > > |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
On Sun, 19 Jan 2014, Levente Uzonyi wrote:
> > > On Sun, 19 Jan 2014, David T. Lewis wrote: > >> On Sun, Jan 19, 2014 at 06:06:37PM +0100, Levente Uzonyi wrote: >>> On Sun, 19 Jan 2014, Nicolas Cellier wrote: >>> >>>> It's a bug. Let's remove the optimizatoin. If we really want it, this >>>> could be a implemented like >>>> >>>> WriteStream>>sharedContents? | sz | ^(sz := position max: readLimit) = >>>> collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 to: >>>> sz] >>> >>> #new:streamContents: is about optimization, nothing else. I think it's a >>> better solution to set the stream to the end before comparing the position >>> with the size, because I don't see any other possible use for >>> #sharedContents. >> >> Do you mean something like this? >> >> new: newSize streamContents: blockWithArg >> | stream p endPos | >> stream := WriteStream on: (self new: newSize). >> blockWithArg value: stream. >> p := stream position. > > It doesn't matter what the current position of the stream is. If there's data > after the current position, then #contents will return it. So I think this is Um, it seems like #contents only cares about position, but not readLimit (even though it updates readLimit), which may or may not be a bug... Levente > how the method should be: > > new: newSize streamContents: blockWithArg > > | stream | > stream := WriteStream on: (self new: newSize). > blockWithArg value: stream. > stream setToEnd position = newSize > ifTrue: [ ^stream originalContents ] > ifFalse: [ ^stream contents ] > > > Levente > >> endPos := stream setToEnd position. >> p = endPos >> ifTrue: [p = newSize >> ifTrue: [^ stream originalContents]] >> ifFalse: [stream position: p]. >> ^ stream contents >> >> >> I'm not sure what that would do to the performance gains. >> >> Dave >> >> >> > > |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
On Sun, 19 Jan 2014, Levente Uzonyi wrote:
> On Sun, 19 Jan 2014, Levente Uzonyi wrote: > >> >> >> On Sun, 19 Jan 2014, David T. Lewis wrote: >> >>> On Sun, Jan 19, 2014 at 06:06:37PM +0100, Levente Uzonyi wrote: >>>> On Sun, 19 Jan 2014, Nicolas Cellier wrote: >>>> >>>>> It's a bug. Let's remove the optimizatoin. If we really want it, this >>>>> could be a implemented like >>>>> >>>>> WriteStream>>sharedContents? | sz | ^(sz := position max: readLimit) = >>>>> collection size ifTrue: [collection] ifFalse: [collection copyFrom: 1 >>>>> to: >>>>> sz] >>>> >>>> #new:streamContents: is about optimization, nothing else. I think it's a >>>> better solution to set the stream to the end before comparing the >>>> position >>>> with the size, because I don't see any other possible use for >>>> #sharedContents. >>> >>> Do you mean something like this? >>> >>> new: newSize streamContents: blockWithArg >>> | stream p endPos | >>> stream := WriteStream on: (self new: newSize). >>> blockWithArg value: stream. >>> p := stream position. >> >> It doesn't matter what the current position of the stream is. If there's >> data after the current position, then #contents will return it. So I think >> this is > > Um, it seems like #contents only cares about position, but not readLimit > (even though it updates readLimit), which may or may not be a bug... Well, it's not a bug, but me being a bit sleepy. All we need to do is to check if the stream is still using the original collection. new: newSize streamContents: blockWithArg | stream collection | collection := self new: newSize. stream := WriteStream on: collection. blockWithArg value: stream. (stream originalContents == collection and: [ stream position = newSize ]) ifTrue: [ ^collection ] ifFalse: [ ^stream contents ] Or another variant, which checks the size of #originalContents. new: newSize streamContents: blockWithArg | stream originalContents | stream := WriteStream on: (self new: newSize). blockWithArg value: stream. originalContents := stream originalContents. (originalContents size = newSize and: [ stream position = newSize ]) ifTrue: [ ^originalContents ] ifFalse: [ ^stream contents ] Levente > > > Levente > >> how the method should be: >> >> new: newSize streamContents: blockWithArg >> >> | stream | >> stream := WriteStream on: (self new: newSize). >> blockWithArg value: stream. >> stream setToEnd position = newSize >> ifTrue: [ ^stream originalContents ] >> ifFalse: [ ^stream contents ] >> >> >> Levente >> >>> endPos := stream setToEnd position. >>> p = endPos >>> ifTrue: [p = newSize >>> ifTrue: [^ stream originalContents]] >>> ifFalse: [stream position: p]. >>> ^ stream contents >>> >>> >>> I'm not sure what that would do to the performance gains. >>> >>> Dave >>> >>> >>> >> >> > > |
Re: The Inbox: CollectionsTests-dtl.212.mcz
|
|
2014/1/19 Levente Uzonyi <[hidden email]>
Or to profit by luck: we pre-allocated 100, the originalContents grew to 200 but precisely fits stream position: originalContents size = stream position ifTrue: [^originalContents] Levente |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |