The Inbox: Files-dtl.184.mcz
Locked
 
|
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 ] ]! |
Re: The Inbox: Files-dtl.184.mcz
|
|
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. 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:. 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. 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. Levente On Fri, 25 Oct 2019, [hidden email] 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 ] ]! |
Re: The Inbox: Files-dtl.184.mcz
|
|
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? > 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. > > 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. > > 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 primitive 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, [hidden email] 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 ] ]! > |
Re: The Inbox: Files-dtl.184.mcz
|
|
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, [hidden email] 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 ] ]! >> |
Re: The Inbox: Files-dtl.184.mcz
|
|
Hi Levente,
On Sun, Oct 27, 2019 at 08:02:13PM +0100, Levente Uzonyi wrote: > On Sat, 26 Oct 2019, David T. Lewis wrote: > > >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)" > Indeed, that makes it very clear! > > > >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]. > I just tried your suggesting [ByteArray becomeCompact], and it works fine. As you say, there is plenty of space in the compact classes array, and this changes the total number of entries from 13 to 14 in the V3 image. Thank you for your patient explanations. I moved Files-dtl.184 to the treated inbox. Dave |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |