Strange behavior of blocks
Dear all, using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem: |state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2]. and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer: |state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2]. and if you move the state variable inside the block, it works again : | values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2]. Same problem in Pharo 3.0 or Pharo 4.0 Regards, -- Serge Stinckwich UCBN & UMI UMMISCO 209 (IRD/UPMC) Every DSL ends up being Smalltalk http://www.doesnotunderstand.org/
On 01 Aug 2014, at 3:20 , Serge Stinckwich <serge.stinckwich@gmail.com> wrote:
Dear all,
using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ
I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem:
|state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2].
and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer:
|state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
and if you move the state variable inside the block, it works again :
| values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
Same problem in Pharo 3.0 or Pharo 4.0
Regards,
Speaking of which, when trying to inspect the array, itâs incredibly slow⦠The main reason for this? BasicIndexedEyeElement >> hash ^super hash bitXor: index hash The super implementation? AbstractEyeElement >> hash ^host hash Guess what the host of an indexedEyeElement is? Thatâs right, the collection⦠So every time it tries to find the cached icon of a line (which is, 10-20 times per refresh cycle depending on the view), it iterates through the 100000 element array to calculate its hash⦠Iâd recommend either removing the super call from BasicIndexedEyeElement, or tell the AbstractEyeElement to use the identityHash of its host instead. Bonus: It also invokes a #DNU hander every time, since DictionaryValueHolder >> at:ifAbsentPut: is implemented self at: key ifAbsent: []. instead of value at: key ifAbsent: []. Code quality progress. Yay. Cheers, Henry
2014-08-01 16:11 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 01 Aug 2014, at 3:20 , Serge Stinckwich <serge.stinckwich@gmail.com> wrote:
Dear all,
using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ
I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem:
|state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2].
and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer:
|state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
and if you move the state variable inside the block, it works again :
| values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
Same problem in Pharo 3.0 or Pharo 4.0
Regards,
Speaking of which, when trying to inspect the array, itâs incredibly slow⦠The main reason for this? BasicIndexedEyeElement >> hash ^*super* hash bitXor: index hash
The super implementation? AbstractEyeElement >> hash ^host hash
Guess what the host of an indexedEyeElement is? Thatâs right, the collection⦠So every time it tries to find the cached icon of a line (which is, 10-20 times per refresh cycle depending on the view), it iterates through the 100000 element array to calculate its hashâ¦
Iâd recommend either removing the super call from BasicIndexedEyeElement, or tell the AbstractEyeElement to use the identityHash of its host instead.
Right, current hash is bad and long to compute. I think we should use instead BasicIndexedEyeElement >> hash ^ index hashMultiply host identityHash and index hash may lead to many collisions... What do you think ?
Bonus: It also invokes a #DNU hander every time, since DictionaryValueHolder >> at:ifAbsentPut: is implemented *self* at: key ifAbsent: []. instead of *value* at: key ifAbsent: [].
Your code looks obviously better.
I'm making a slice for that.
Code quality progress. Yay.
Thanks for your contribution. Clement.
Cheers, Henry
On 03 Aug 2014, at 5:55 , Clément Bera <bera.clement@gmail.com> wrote:
Right, current hash is bad and long to compute.
I think we should use instead BasicIndexedEyeElement >> hash ^ index hashMultiply
host identityHash and index hash may lead to many collisions... What do you think ?
I agree thereâs not much value in including host in the hash for the use case of the icon lookup cache, since those were per host anyways (iirc). Index hash will be a sequential number, I havenât checked the usage closely to see if itâll be a problem wrt. collisions, but removal at least might be slow (if thatâs ever done) due to lots of scanning needed to find the nil slot. Using hashMultiply seems a good precaution. Cheers, Henry
2014-08-04 8:50 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 03 Aug 2014, at 5:55 , Clément Bera <bera.clement@gmail.com> wrote:
Right, current hash is bad and long to compute.
I think we should use instead BasicIndexedEyeElement >> hash ^ index hashMultiply
host identityHash and index hash may lead to many collisions... What do you think ?
I agree thereâs not much value in including host in the hash for the use case of the icon lookup cache, since those were per host anyways (iirc). Index hash will be a sequential number, I havenât checked the usage closely to see if itâll be a problem wrt. collisions, but removal at least might be slow (if thatâs ever done) due to lots of scanning needed to find the nil slot. Using hashMultiply seems a good precaution.
Cheers, Henry
And as far as I know, the icon lookup isn't needed for the inspector, as it does not show any icons at all!
On 04 Aug 2014, at 9:31 , Nicolai Hess <nicolaihess@web.de> wrote:
2014-08-04 8:50 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 03 Aug 2014, at 5:55 , Clément Bera <bera.clement@gmail.com> wrote:
Right, current hash is bad and long to compute.
I think we should use instead BasicIndexedEyeElement >> hash ^ index hashMultiply
host identityHash and index hash may lead to many collisions... What do you think ?
I agree thereâs not much value in including host in the hash for the use case of the icon lookup cache, since those were per host anyways (iirc). Index hash will be a sequential number, I havenât checked the usage closely to see if itâll be a problem wrt. collisions, but removal at least might be slow (if thatâs ever done) due to lots of scanning needed to find the nil slot. Using hashMultiply seems a good precaution.
Cheers, Henry
And as far as I know, the icon lookup isn't needed for the inspector, as it does not show any icons at all!
Well, it still needs to figure out there is no icon to be displayed for the entry⦠In that process, it caches the result (nil), and that cache is too expensive to query later on, due to the expensive hash calculation needed to find an entry. Cheers, Henry
2014-08-04 9:57 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 04 Aug 2014, at 9:31 , Nicolai Hess <nicolaihess@web.de> wrote:
2014-08-04 8:50 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 03 Aug 2014, at 5:55 , Clément Bera <bera.clement@gmail.com> wrote:
Right, current hash is bad and long to compute.
I think we should use instead BasicIndexedEyeElement >> hash ^ index hashMultiply
host identityHash and index hash may lead to many collisions... What do you think ?
I agree thereâs not much value in including host in the hash for the use case of the icon lookup cache, since those were per host anyways (iirc). Index hash will be a sequential number, I havenât checked the usage closely to see if itâll be a problem wrt. collisions, but removal at least might be slow (if thatâs ever done) due to lots of scanning needed to find the nil slot. Using hashMultiply seems a good precaution.
Cheers, Henry
And as far as I know, the icon lookup isn't needed for the inspector, as it does not show any icons at all!
Well, it still needs to figure out there is no icon to be displayed for the entry⦠In that process, it caches the result (nil), and that cache is too expensive to query later on, due to the expensive hash calculation needed to find an entry.
Cheers, Henry
Thats the reason I created the issue 13315 NewList without icons (13315 <https://pharo.fogbugz.com/default.asp?13315>) We need a way to tell NewList to not use and therefore don't try to lookup icons, if the client (the inspector) don't provide any icons at all. Maybe NewList should not hold an icon cache at all, but rely on the icon provider for proper caching. Nicolai
Hello, Nice way to reproduce easily the SmallInteger bug :) | state values | values := (1 to: 100000) collect: [ :t | state := { t . t + 1 . t + 2 }]. values select: [ :e | e isInteger ] This bug was present in some VMs and it is fixed in the latest VMs. The stable Mac VM still have the bug. If you need a VM without the bug, go there: http://files.pharo.org/vm/pharo/30/ Click on your OS, then select the zip file named 'latest.zip' and download it. The latest VM does not have the bug. 2014-08-01 15:20 GMT+02:00 Serge Stinckwich <serge.stinckwich@gmail.com>:
Dear all,
using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ
I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem:
|state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2].
and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer:
|state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
and if you move the state variable inside the block, it works again :
| values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
Same problem in Pharo 3.0 or Pharo 4.0
Regards, -- Serge Stinckwich UCBN & UMI UMMISCO 209 (IRD/UPMC) Every DSL ends up being Smalltalk http://www.doesnotunderstand.org/
On Fri, Aug 1, 2014 at 4:43 PM, Clément Bera <bera.clement@gmail.com> wrote:
Hello,
Nice way to reproduce easily the SmallInteger bug :)
| state values | values := (1 to: 100000) collect: [ :t | state := { t . t + 1 . t + 2 }]. values select: [ :e | e isInteger ]
Maybe we should convert this as a test in order to avoid any regression in the future ?
This bug was present in some VMs and it is fixed in the latest VMs.
Ok, thank you Clément. I was not aware of this bug until now.
The stable Mac VM still have the bug.
If you need a VM without the bug, go there:
http://files.pharo.org/vm/pharo/30/
Click on your OS, then select the zip file named 'latest.zip' and download it.
The latest VM does not have the bug.
I'm using PharoLauncher and apparently the mac OS X version still use the buggy VM. Thank you. Regards, -- Serge Stinckwich UCBN & UMI UMMISCO 209 (IRD/UPMC) Every DSL ends up being Smalltalk http://www.doesnotunderstand.org/
thanks clement! Could you also have a look at the suggestions henrik mentioned? Stef On 1/8/14 16:43, Clément Bera wrote:
Hello,
Nice way to reproduce easily the SmallInteger bug :)
| state values | values := (1 to: 100000) collect: [ :t | state := { t . t + 1 . t + 2 }]. values select: [ :e | e isInteger ]
This bug was present in some VMs and it is fixed in the latest VMs.
The stable Mac VM still have the bug.
If you need a VM without the bug, go there:
http://files.pharo.org/vm/pharo/30/
Click on your OS, then select the zip file named 'latest.zip' and download it.
The latest VM does not have the bug.
2014-08-01 15:20 GMT+02:00 Serge Stinckwich <serge.stinckwich@gmail.com <mailto:serge.stinckwich@gmail.com>>:
Dear all,
using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ <https://groups.google.com/forum/#%21topic/scismalltalk/HmGpTkzLOdQ>
I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem:
|state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2].
and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer:
|state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
and if you move the state variable inside the block, it works again :
| values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
Same problem in Pharo 3.0 or Pharo 4.0
Regards, -- Serge Stinckwich UCBN & UMI UMMISCO 209 (IRD/UPMC) Every DSL ends up being Smalltalk http://www.doesnotunderstand.org/
2014-08-01 22:40 GMT+02:00 stepharo <stepharo@free.fr>:
thanks clement! Could you also have a look at the suggestions henrik mentioned?
I'll have a look on monday.
Stef
On 1/8/14 16:43, Clément Bera wrote:
Hello,
Nice way to reproduce easily the SmallInteger bug :)
| state values | values := (1 to: 100000) collect: [ :t | state := { t . t + 1 . t + 2 }]. values select: [ :e | e isInteger ]
This bug was present in some VMs and it is fixed in the latest VMs.
The stable Mac VM still have the bug.
If you need a VM without the bug, go there:
http://files.pharo.org/vm/pharo/30/
Click on your OS, then select the zip file named 'latest.zip' and download it.
The latest VM does not have the bug.
2014-08-01 15:20 GMT+02:00 Serge Stinckwich <serge.stinckwich@gmail.com>:
Dear all,
using Sci-Smalltalk, we found with Natalia that sometimes blocks have strange behavior : https://groups.google.com/forum/#!topic/scismalltalk/HmGpTkzLOdQ
I was able to create a more simpler example using only Pharo. If you try, the following expression, there is no problem:
|state values | values := (1 to: 10000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 10000) do:[:i | (values at:i) at:2].
and you do more iterations, an error appears (Instances of SmallInteger are not indexable), because from time to time, an array is replaced by an integer:
|state values | values := (1 to: 100000) collect: [ : t| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
and if you move the state variable inside the block, it works again :
| values | values := (1 to: 100000) collect: [ : t| |state| state := { t. t+1. t+2.}]. (1 to: 100000) do:[:i | (values at:i) at:2].
Same problem in Pharo 3.0 or Pharo 4.0
Regards, -- Serge Stinckwich UCBN & UMI UMMISCO 209 (IRD/UPMC) Every DSL ends up being Smalltalk http://www.doesnotunderstand.org/
participants (5)
-
Clément Bera -
Henrik Johansen -
Nicolai Hess -
Serge Stinckwich -
stepharo