Hi Dave,

On Sat, 26 Oct 2019, David T. Lewis wrote:

> Levente,
>
> Thanks for looking at this :-)
>
> On Fri, Oct 25, 2019 at 03:22:32PM +0200, Levente Uzonyi wrote:
>> Hi Dave,
>> 
>> While error handling seems to be a straigforward fix, the reason why 
>> #adoptInstance: is used there is to:
>> 1. be as quick as possible
>> 2. avoid garbage creation (to achieve 1)
>> An error handler works against those goals.
>
> OK. But a question, off topic and just for my own understanding - I was
> expecting that the error handler would mainly add overhead in the case
> of handling a failure (the V3 image case), and would add little overhead
> in the normal case (Spur) for which the adoptInstance: is successful.
> I was not worried about performance in the V3 case (because #binary
> is not frequently called so AFAICT it does not matter), so this seemed
> like a reasonable solution.
>
> From a performance perspective, is it better to avoid using on:do: even
> if one expects the block to succeed in the common case?

Yes. The block has to be created, the error handler has to be initialized, 
even if there will be no error.
Here's a small benchmark I just came up with:

({
 	[ "Empty code. Result should be subtracted from the other results."
 		| s |
 		s := ByteString new: 1.
 		1 to: 1000000 do: [ :i | ] ] timeToRun.
 	Smalltalk garbageCollect.
 	[ "#adoptInstance:"
 		| s |
 		s := ByteString new: 1.
 		1 to: 1000000 do: [ :i |
 			ByteArray adoptInstance: s.
 			ByteString adoptInstance: s. ] ] timeToRun.
 	Smalltalk garbageCollect.
 	[ "#asByteArray"
 		| s |
 		s := ByteString new: 1.
 		1 to: 1000000 do: [ :i |
 			s := s asByteArray.
 			s := s asString ] ] timeToRun.
 	Smalltalk garbageCollect.
 	[ "#adoptInstance: with error handler for ByteString -> ByteArray conversion"
 		| s |
 		s := ByteString new: 1.
 		1 to: 1000000 do: [ :i |
 			[ ByteArray adoptInstance: s ] on: Error do: [ s := s asByteArray ].
 			ByteString adoptInstance: s ] ] timeToRun.
} withIndexCollect: [ :each :index | index odd ifTrue: [ each ] ]) reject: #isNil

"==> #(2 33 75 76)"

>
>> Using pure #asByteArray, which is even more straightforward, would be a 
>> better choice, as it would be faster and would create no or less garbage 
>> than an error handler. If you have a look at the method's history, you'll 
>> see that was used before #adoptInstance:.
>
> You're right, the 'eem 5/27/2014' version of the method would be a
> better solution overall. But if using adoptInstance is important,
> then we should just leave things as they are.

I wouldn't say it's that important. I changed it when I came across some 
code that heavily swapped between ascii and binary modes, which was 
noticable in the profiler.

>
>> 
>> There is also the complementary method, #ascii, which does the same 
>> conversion but to ByteString instead of to ByteArray. I suppose that 
>> method also doesn't work on V3.
>
> The #ascii implementation is not a concern. ByteString can adopt an
> instance of ByteArray on V3.

Okay, I thought #adoptInstance: was not supported in V3.
Since it is supported, I presume the problem is that ByteArray is not 
compact while ByteString is compact. So, if ByteArray were compact, then I 
think #adoptInstance: would work.
IIRC more than half of #compactClassesArray is empty, so perhaps the 
easiest fix is to make ByteArray compact. That should speed up other 
things as well. It was a while ago I used a V3 image, but I think it's as 
simple as evaluating [ByteArray becomeCompact].

Levente

>
>> 
>> IIRC, the V3 VMs support primitive 115 (#primitiveChangeClassTo:). If they 
>> do, I'd rather change #adoptInstance:'s fallback code to try 
>> #primitiveChangeClassTo: (preferably only on V3 to avoid any side 
>> effects). That way other users of #adoptInstance: could work on V3.
>
> Both primitives have the same constraint on V3. And, in fact, the
> earlier fallback code (hsj 2/16/2011) does what you suggest:
>
> Behavior>>adoptInstance: anInstance
> 	"Change the class of anInstance to me.
> 	Primitive (found in Cog and new VMs)  follows the same rules as primitiveChangeClassTo:, but returns the class rather than the modified instance"
>
> 	<primitive: 160 error: ec>
> 	anInstance primitiveChangeClassTo: self basicNew.
> 	^self
>
> But this does not help because primtitive 115 fails for the same reason
> as primitive 160.
>
> Again, thank you for looking at this. This is not an important problem
> to fix, just a bit of an annoyance for me in keeping track of the V3/Spur
> differences.
>
> Dave
>
>> 
>> Levente
>> 
>> On Fri, 25 Oct 2019, commits at source.squeak.org wrote:
>> 
>> >David T. Lewis uploaded a new version of Files to project The Inbox:
>> >http://source.squeak.org/inbox/Files-dtl.184.mcz
>> >
>> >==================== Summary ====================
>> >
>> >Name: Files-dtl.184
>> >Author: dtl
>> >Time: 24 October 2019, 10:15:34.856587 pm
>> >UUID: cfa02841-644b-49cb-b3f2-a0eb1b36707b
>> >Ancestors: Files-pre.183
>> >
>> >Provide error handling for #adoptInstance in StandardFileStream>>binary to 
>> >provide pre-Spur behavior when running on a V3 image. Prior to Spur, class 
>> >ByteArray cannot adopt an instance of ByteString, so use #asByteArray 
>> >instead.
>> >
>> >Resolves the only remaining difference for package Files when running on 
>> >either Spur or V3 images. The packages with significant Spur/V3 
>> >differences (excluding unit tests) are now Collections, Compiler, Kernel, 
>> >and System. See www.squeaksource.com/TrunkUpdateStreamV3 for verification.
>> >
>> >=============== Diff against Files-pre.183 ===============
>> >
>> >Item was changed:
>> > ----- Method: StandardFileStream>>binary (in category 
>> > 'properties-setting') -----
>> > binary
>> > 	"Read and/or write in binary mode."
>> > 	buffer1
>> > 		ifNil: [ buffer1 := ByteArray new: 1 ]
>> >+ 		ifNotNil: [ [ ByteArray adoptInstance: buffer1 ]
>> >+ 						on: Error "V3 image"
>> >+ 						do: [ buffer1 := ByteArray 
>> >new: 1 ] ].
>> >+ 	collection ifNotNil: [ [ ByteArray adoptInstance: collection ]
>> >+ 							on: Error
>> >+ 							do: [ collection := 
>> >collection asByteArray ] ].
>> >- 		ifNotNil: [ ByteArray adoptInstance: buffer1 ].
>> >- 	collection ifNotNil: [ ByteArray adoptInstance: collection ].
>> > 	lastWritten ifNotNil:
>> > 		[ lastWritten isCharacter ifTrue: [ lastWritten := 
>> > 		lastWritten asInteger ] ]!
>>