Squeak 4.6: Graphics-mt.316.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Squeak 4.6: Graphics-mt.316.mcz

commits-2
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  ].
  !


Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Levente Uzonyi-2
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  ].
>  !
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

marcel.taeumel
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
Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

marcel.taeumel
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
Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Levente Uzonyi-2
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.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Nicolas Cellier
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 spur

2015-10-20 11:58 GMT+02:00 Levente Uzonyi <[hidden email]>:
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.






Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

marcel.taeumel
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
Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Nicolas Cellier
 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.




Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Levente Uzonyi-2
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.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Eliot Miranda-2
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:
 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


Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

marcel.taeumel
In reply to this post by Levente Uzonyi-2
Sounds reasonable. :)

Best,
Marcel
Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Levente Uzonyi-2
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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Squeak 4.6: Graphics-mt.316.mcz

Nicolas Cellier
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.

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