Squeak 4.6: Graphics-mt.316.mcz
Locked
 
|
Marcel Taeumel uploaded a new version of Graphics to project Squeak 4.6:
http://source.squeak.org/squeak46/Graphics-mt.316.mcz ==================== Summary ==================== Name: Graphics-mt.316 Author: mt Time: 20 October 2015, 11:40:19.755 am UUID: 41cf59e1-6149-7e4b-9d40-485a1f4798ef Ancestors: Graphics-mt.313 Fixes loading of PNG files for recent (> #3397) v3 CogVMs. There seems to be a bug in the VM that, at some point, manifests itself by replacing z, which is the current ZLib stream, with self, which is the PNGReadWriter. Any indirection with a BlockClosure will do. I chose to create an interval and then calling #do: on that interval. Another way would be to wrap the body/argument of #to:do: into another block like this: 0 to: height-1 do: [:y | [ ... ] value ]. This fix might affect performance a tiny little bit and might be removed as soon as the VM bug gets fixed. Note that there might be other places in the code affected by this bug. =============== Diff against Graphics-mt.313 =============== Item was changed: ----- Method: PNGReadWriter>>processNonInterlaced (in category 'chunks') ----- processNonInterlaced | z filter temp copyMethod debug | debug := self debugging. copyMethod := #(copyPixelsGray: nil copyPixelsRGB: copyPixelsIndexed: copyPixelsGrayAlpha: nil copyPixelsRGBA:) at: colorType+1. debug ifTrue: [ Transcript cr; nextPutAll: 'NI chunk size='; print: idatChunkStream position ]. z := ZLibReadStream on: idatChunkStream originalContents from: 1 to: idatChunkStream position. prevScanline := ByteArray new: bytesPerScanline. thisScanline := ByteArray new: bytesPerScanline. + (0 to: height-1) do: [ :y | - 0 to: height-1 do: [ :y | filter := z next. debug ifTrue:[filtersSeen add: filter]. thisScanline := z next: bytesPerScanline into: thisScanline startingAt: 1. (debug and: [ thisScanline size < bytesPerScanline ]) ifTrue: [ Transcript nextPutAll: ('wanted {1} but only got {2}' format: { bytesPerScanline. thisScanline size }); cr ]. filter = 0 ifFalse:[self filterScanline: filter count: bytesPerScanline]. self perform: copyMethod with: y. temp := prevScanline. prevScanline := thisScanline. thisScanline := temp. ]. z atEnd ifFalse:[self error:'Unexpected data']. debug ifTrue: [Transcript nextPutAll: ' compressed size='; print: z position ]. ! |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
I don't think it's a good idea to change any code to work around a
temporary VM bug. I have seen this error in other cases as well (sockets) and I decided to revert the VM to 3397. I haven't reported the bug yet, because I don't have a reproducible case. The only symptom I saw is that the instance variables get mixed with temporaries (including method arguments). Levente On Tue, 20 Oct 2015, [hidden email] wrote: > Marcel Taeumel uploaded a new version of Graphics to project Squeak 4.6: > http://source.squeak.org/squeak46/Graphics-mt.316.mcz > > ==================== Summary ==================== > > Name: Graphics-mt.316 > Author: mt > Time: 20 October 2015, 11:40:19.755 am > UUID: 41cf59e1-6149-7e4b-9d40-485a1f4798ef > Ancestors: Graphics-mt.313 > > Fixes loading of PNG files for recent (> #3397) v3 CogVMs. > > There seems to be a bug in the VM that, at some point, manifests itself by replacing z, which is the current ZLib stream, with self, which is the PNGReadWriter. Any indirection with a BlockClosure will do. I chose to create an interval and then calling #do: on that interval. Another way would be to wrap the body/argument of #to:do: into another block like this: > > 0 to: height-1 do: [:y | [ ... ] value ]. > > This fix might affect performance a tiny little bit and might be removed as soon as the VM bug gets fixed. > > Note that there might be other places in the code affected by this bug. > > =============== Diff against Graphics-mt.313 =============== > > Item was changed: > ----- Method: PNGReadWriter>>processNonInterlaced (in category 'chunks') ----- > processNonInterlaced > | z filter temp copyMethod debug | > debug := self debugging. > copyMethod := #(copyPixelsGray: nil copyPixelsRGB: copyPixelsIndexed: > copyPixelsGrayAlpha: nil copyPixelsRGBA:) at: colorType+1. > debug ifTrue: [ Transcript cr; nextPutAll: 'NI chunk size='; print: idatChunkStream position ]. > z := ZLibReadStream > on: idatChunkStream originalContents > from: 1 > to: idatChunkStream position. > prevScanline := ByteArray new: bytesPerScanline. > thisScanline := ByteArray new: bytesPerScanline. > + (0 to: height-1) do: [ :y | > - 0 to: height-1 do: [ :y | > filter := z next. > debug ifTrue:[filtersSeen add: filter]. > thisScanline := z next: bytesPerScanline into: thisScanline startingAt: 1. > (debug and: [ thisScanline size < bytesPerScanline ]) ifTrue: [ Transcript nextPutAll: ('wanted {1} but only got {2}' format: { bytesPerScanline. thisScanline size }); cr ]. > filter = 0 ifFalse:[self filterScanline: filter count: bytesPerScanline]. > self perform: copyMethod with: y. > temp := prevScanline. > prevScanline := thisScanline. > thisScanline := temp. > ]. > z atEnd ifFalse:[self error:'Unexpected data']. > debug ifTrue: [Transcript nextPutAll: ' compressed size='; print: z position ]. > ! > > > |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
In reply to this post by commits-2
Hi Eliot, :)
Tobias reminded me that this might be a bug in the JIT because it only occurs if y is 42. This method is rather long and has a lot of temporaries. Additionally, the block argument of a #to:do: call will be inlined. Hope that helps. Best, Marcel |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by Levente Uzonyi-2
Usually, I would agree. But we did already release Squeak 4.6 and thus have to react quickly if it does not messes things up, which this fix does not. :)
Best, Marcel |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
The release comes with the 3397 VM, which works fine.
Your fix works the bug around in a single method, but I'm sure there are hundreds of places where the issue can occur. Levente On Tue, 20 Oct 2015, marcel.taeumel wrote: > Usually, I would agree. But we did already release Squeak 4.6 and thus have > to react quickly if it does not messes things up, which this fix does not. > :) > > Best, > Marcel > > > > -- > View this message in context: http://forum.world.st/Squeak-4-6-Graphics-mt-316-mcz-tp4856712p4856721.html > Sent from the Squeak - Dev mailing list archive at Nabble.com. > > |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
It works OK with stack VM, so I confirm it's a JIT thing So far I saw it only in cog v3, but not in cog spur2015-10-20 11:58 GMT+02:00 Levente Uzonyi <[hidden email]>: The release comes with the 3397 VM, which works fine. |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by Levente Uzonyi-2
And if you update the VM, it will break.
And if you update the image, now, it will work again. ;-) Well, here is the thing: I am not able to fix that VM/JIT bug although I would rather like to. If you can fix it, please go ahead. If Eliot finds time for it -- well, it works in Spur and Stack versions -- he will surely do it. Until then, we should fix it if possible to improve user experience. If you find any other of those hundreds of places, please consider fixing them as well. Not into trunk, but Squeak46 branch. This VM bug has been known since mid of September at least: http://forum.world.st/Non-Spur-CogVM-3427-and-3410-cannot-load-PNG-files-anymore-td4850308.html Nothing has happened yet. Best, Marcel |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
BTW, I had a fairly reproducible case, didn't I send it to this list? Ah no, it was pharo... ScaledDecimalTests>>testCoercion https://www.mail-archive.com/pharo-dev@.../msg31997.html 2015-10-20 15:26 GMT+02:00 marcel.taeumel <[hidden email]>: And if you update the VM, it will break. |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by marcel.taeumel
Wouldn't it be better to just warn the users if they're using a VM with
broken JIT? For example adding the following to one of the #startUp: method: (Smalltalk vm isRunningCogit and: [ Smalltalk vm platformSourceVersion asInteger > 3397 ]) ifTrue: [ Warning signal: 'The VM you''re currently using has a bug which may cause unexpected errors in some cases.\Please consider downgrading to version 3397.' withCRs ]. Levente On Tue, 20 Oct 2015, marcel.taeumel wrote: > And if you update the VM, it will break. > And if you update the image, now, it will work again. ;-) > > Well, here is the thing: I am not able to fix that VM/JIT bug although I > would rather like to. If you can fix it, please go ahead. If Eliot finds > time for it -- well, it works in Spur and Stack versions -- he will surely > do it. > > Until then, we should fix it if possible to improve user experience. If you > find any other of those hundreds of places, please consider fixing them as > well. Not into trunk, but Squeak46 branch. > > This VM bug has been known since mid of September at least: > http://forum.world.st/Non-Spur-CogVM-3427-and-3410-cannot-load-PNG-files-anymore-td4850308.html > > Nothing has happened yet. > > Best, > Marcel > > > > -- > View this message in context: http://forum.world.st/Squeak-4-6-Graphics-mt-316-mcz-tp4856712p4856747.html > Sent from the Squeak - Dev mailing list archive at Nabble.com. > > |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by Nicolas Cellier
Nicolas, what's (where's) the reproducible case? On Tue, Oct 20, 2015 at 8:52 AM, Nicolas Cellier <[hidden email]> wrote:
_,,,^..^,,,_ best, Eliot |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by Levente Uzonyi-2
Sounds reasonable. :)
Best, Marcel |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
In reply to this post by Eliot Miranda-2
Running that test case on the latest V3 CogVM (3427) should give you a
debugger where the receiver got mixed up with some other variable. Levente On Fri, 23 Oct 2015, Eliot Miranda wrote: > Nicolas, > what's (where's) the reproducible case? > > On Tue, Oct 20, 2015 at 8:52 AM, Nicolas Cellier <[hidden email]> wrote: > BTW, I had a fairly reproducible case, didn't I send it to this list? > Ah no, it was pharo... > > ScaledDecimalTests>>testCoercion > > https://www.mail-archive.com/pharo-dev@.../msg31997.html > > 2015-10-20 15:26 GMT+02:00 marcel.taeumel <[hidden email]>: > And if you update the VM, it will break. > And if you update the image, now, it will work again. ;-) > > Well, here is the thing: I am not able to fix that VM/JIT bug although I > would rather like to. If you can fix it, please go ahead. If Eliot finds > time for it -- well, it works in Spur and Stack versions -- he will surely > do it. > > Until then, we should fix it if possible to improve user experience. If you > find any other of those hundreds of places, please consider fixing them as > well. Not into trunk, but Squeak46 branch. > > This VM bug has been known since mid of September at least: > http://forum.world.st/Non-Spur-CogVM-3427-and-3410-cannot-load-PNG-files-anymore-td4850308.html > > Nothing has happened yet. > > Best, > Marcel > > > > -- > View this message in context: http://forum.world.st/Squeak-4-6-Graphics-mt-316-mcz-tp4856712p4856747.html > Sent from the Squeak - Dev mailing list archive at Nabble.com. > > > > > > > > > -- > _,,,^..^,,,_ > best, Eliot > > |
Re: Squeak 4.6: Graphics-mt.316.mcz
|
|
Hi Eliot, the test case is :ScaledDecimalTest new testCoercion 2015-10-24 15:54 GMT+02:00 Levente Uzonyi <[hidden email]>: Running that test case on the latest V3 CogVM (3427) should give you a debugger where the receiver got mixed up with some other variable. |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |