speed up with:do: by not checking size
with:do: on SequencableCollection checks that both collections have the same size (it raises an Error if not). with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)] I think we should not do that: 1) it is slow 2) when the other is larger, is would work and just omit the rest 3) if it is smaller, you will get an error anyway There are just three methods doing such a size check: with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception) Is there a reason why this makes sense? I added an issue with a slice to clean it up: https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz... Marcus
2014-09-03 14:40 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
with:do: on SequencableCollection checks that both collections have the same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow
Are you sure about that one? The check is a constant cost disregarding the collection length, and, anyway, self size is reused just after that.
2) when the other is larger, is would work and just omit the rest
3) if it is smaller, you will get an error anyway
I have a feeling I'm not in Pharo anymore, but in R instead ;) Thierry
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
On Wed, Sep 3, 2014 at 7:54 AM, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2014-09-03 14:40 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
with:do: on SequencableCollection checks that both collections have the
same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow
Are you sure about that one? The check is a constant cost disregarding the collection length, and, anyway, self size is reused just after that.
2) when the other is larger, is would work and just omit the rest
3) if it is smaller, you will get an error anyway
I have a feeling I'm not in Pharo anymore, but in R instead ;)
+2. There are clear safety advantages in the check. If you want the "faster" protocol why not define withSomeOf:do: or some such?
Thierry
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
-- best, Eliot
On 3 sept. 2014, at 17:26, Eliot Miranda <eliot.miranda@gmail.com> wrote:
On Wed, Sep 3, 2014 at 7:54 AM, Thierry Goubier <thierry.goubier@gmail.com> wrote:
2014-09-03 14:40 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
with:do: on SequencableCollection checks that both collections have the same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow
Are you sure about that one? The check is a constant cost disregarding the collection length, and, anyway, self size is reused just after that.
2) when the other is larger, is would work and just omit the rest 3) if it is smaller, you will get an error anyway
I have a feeling I'm not in Pharo anymore, but in R instead ;)
+2. There are clear safety advantages in the check. If you want the "faster" protocol why not define withSomeOf:do: or some such?
Like Thierry I'm not sure that omitting the check brings a significant speedup. However, not doing this check follows the robustness principle: "Be conservative in what you do, be liberal in what you accept from others". I would even be more liberal with a: 1 to: (self size min: otherCollection size) do: .... :)
Thierry
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
-- best, Eliot
On Wed, Sep 3, 2014 at 8:37 AM, Camille Teruel <camille.teruel@gmail.com> wrote:
On 3 sept. 2014, at 17:26, Eliot Miranda <eliot.miranda@gmail.com> wrote:
On Wed, Sep 3, 2014 at 7:54 AM, Thierry Goubier <thierry.goubier@gmail.com
wrote:
2014-09-03 14:40 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
with:do: on SequencableCollection checks that both collections have the
same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow
Are you sure about that one? The check is a constant cost disregarding the collection length, and, anyway, self size is reused just after that.
2) when the other is larger, is would work and just omit the rest
3) if it is smaller, you will get an error anyway
I have a feeling I'm not in Pharo anymore, but in R instead ;)
+2. There are clear safety advantages in the check. If you want the "faster" protocol why not define withSomeOf:do: or some such?
Like Thierry I'm not sure that omitting the check brings a significant speedup. However, not doing this check follows the robustness principle: "Be conservative in what you do, be liberal in what you accept from others".
But it violates the "if it ain't broke don't fix it", "it means what it means", and "Smalltalk is a safe language" principles. That safety check finds bugs. If one wants a more relaxed contract, implement a new contract, don't break an existing contract that has stood for years.
I would even be more liberal with a: 1 to: (self size min: otherCollection size) do: .... :)
But you can write that if that's what you mean. But arbitrarily changing existing protocol for weak reasons when _it ain't broke_ is a recipe for chaos. Thierry
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
-- best, Eliot
-- best, Eliot
Like Thierry I'm not sure that omitting the check brings a significant speedup. However, not doing this check follows the robustness principle: "Be conservative in what you do, be liberal in what you accept from others".
But it violates the "if it ain't broke don't fix it", "it means what it means", and "Smalltalk is a safe language" principles. That safety check finds bugs. If one wants a more relaxed contract, implement a new contract, don't break an existing contract that has stood for years.
I would even be more liberal with a: 1 to: (self size min: otherCollection size) do: .... :)
But you can write that if that's what you mean. But arbitrarily changing existing protocol for weak reasons when _it ain't broke_ is a recipe for chaos.
+ 1 Stef
On Wed, Sep 3, 2014 at 8:19 PM, stepharo <stepharo@free.fr> wrote:
Like Thierry I'm not sure that omitting the check brings a
significant speedup. However, not doing this check follows the robustness principle: "Be conservative in what you do, be liberal in what you accept from others".
But it violates the "if it ain't broke don't fix it", "it means what it means", and "Smalltalk is a safe language" principles. That safety check finds bugs. If one wants a more relaxed contract, implement a new contract, don't break an existing contract that has stood for years.
I would even be more liberal with a: 1 to: (self size min: otherCollection size) do: .... :)
But you can write that if that's what you mean. But arbitrarily changing existing protocol for weak reasons when _it ain't broke_ is a recipe for chaos.
Ok, then I will not do it... Marcus
This check is not in a loop so I don't think it is performance critical... I don't know if this change is great. 2014-09-03 16:54 GMT+02:00 Thierry Goubier <thierry.goubier@gmail.com>:
2014-09-03 14:40 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
with:do: on SequencableCollection checks that both collections have the
same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow
Are you sure about that one? The check is a constant cost disregarding the collection length, and, anyway, self size is reused just after that.
2) when the other is larger, is would work and just omit the rest
3) if it is smaller, you will get an error anyway
I have a feeling I'm not in Pharo anymore, but in R instead ;)
Thierry
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
Marcus the contract is important and should be explicit because if you do not check the same size then you may have part of the computation done and not the rest. So I would introduce new methods with explicit names telling that fuzzyWith:do: or soemthing like that. On 3/9/14 14:40, Marcus Denker wrote:
with:do: on SequencableCollection checks that both collections have the same size (it raises an Error if not).
with: otherCollection do: twoArgBlock "Evaluate twoArgBlock with corresponding elements from this collection and otherCollection." otherCollection size = self size ifFalse: [self error: 'otherCollection must be the same size']. 1 to: self size do: [:index | twoArgBlock value: (self at: index) value: (otherCollection at: index)]
I think we should not do that:
1) it is slow 2) when the other is larger, is would work and just omit the rest 3) if it is smaller, you will get an error anyway
There are just three methods doing such a size check:
with:do: with:collect: reverseWith:do: (the last one actually raises a SizeMismatch exception, it is the sole user of that exception)
Is there a reason why this makes sense?
I added an issue with a slice to clean it up:
https://pharo.fogbugz.com/f/cases/13946/speed-up-with-do-by-not-checking-siz...
Marcus
participants (6)
-
Camille Teruel -
Clément Bera -
Eliot Miranda -
Marcus Denker -
stepharo -
Thierry Goubier