A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-kfr.434.mcz
==================== Summary ====================
Name: Graphics-kfr.434 Author: kfr Time: 11 July 2020, 10:18:06.272175 am UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 Ancestors: Graphics-mt.433
I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
=============== Diff against Graphics-mt.433 ===============
Item was changed: ----- Method: Rectangle class>>encompassing: (in category 'instance creation') ----- encompassing: listOfPoints "A number of callers of encompass: should use this method." | topLeft bottomRight | topLeft := bottomRight := nil. listOfPoints do: [:p | topLeft == nil ifTrue: [topLeft := bottomRight := p] ifFalse: [topLeft := topLeft min: p. bottomRight := bottomRight max: p]]. + ^self origin: topLeft corner: bottomRight! - ^ topLeft corner: bottomRight!
Item was changed: ----- Method: Rectangle class>>merging: (in category 'instance creation') ----- merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | listOfRects do: [:r | minX ifNil: [minX := r topLeft x. minY := r topLeft y. maxX := r bottomRight x. maxY := r bottomRight y] ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y. maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]]. + ^ self origin:minX@minY corner: maxX@maxY! - ^ minX@minY corner: maxX@maxY!
Hi Karl,
While both changes look right, they modify the behavior of those methods when their argument is an empty list. Instead of raising an error, they'll return a Rectangle with both fields set to nil. If the argument is really a list - aka SequenceableCollection, I suggest using more specific methods instead of #do: to process the list in order to trigger the error as soon as possible. For example, use #first to get the first element and #allButFirstDo: to iterate over the rest. That would simplify the method as well by making the nil checks unnecessary.
Levente
On Sat, 11 Jul 2020, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-kfr.434.mcz
==================== Summary ====================
Name: Graphics-kfr.434 Author: kfr Time: 11 July 2020, 10:18:06.272175 am UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 Ancestors: Graphics-mt.433
I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
=============== Diff against Graphics-mt.433 ===============
Item was changed: ----- Method: Rectangle class>>encompassing: (in category 'instance creation') ----- encompassing: listOfPoints "A number of callers of encompass: should use this method." | topLeft bottomRight | topLeft := bottomRight := nil. listOfPoints do: [:p | topLeft == nil ifTrue: [topLeft := bottomRight := p] ifFalse: [topLeft := topLeft min: p. bottomRight := bottomRight max: p]].
- ^self origin: topLeft corner: bottomRight!
- ^ topLeft corner: bottomRight!
Item was changed: ----- Method: Rectangle class>>merging: (in category 'instance creation') ----- merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | listOfRects do: [:r | minX ifNil: [minX := r topLeft x. minY := r topLeft y. maxX := r bottomRight x. maxY := r bottomRight y] ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y. maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]].
- ^ self origin:minX@minY corner: maxX@maxY!
- ^ minX@minY corner: maxX@maxY!
Thanks for the suggestion. I uploaded a new version based on it.
Best, Karl
On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi leves@caesar.elte.hu wrote:
Hi Karl,
While both changes look right, they modify the behavior of those methods when their argument is an empty list. Instead of raising an error, they'll return a Rectangle with both fields set to nil. If the argument is really a list - aka SequenceableCollection, I suggest using more specific methods instead of #do: to process the list in order to trigger the error as soon as possible. For example, use #first to get the first element and #allButFirstDo: to iterate over the rest. That would simplify the method as well by making the nil checks unnecessary.
Levente
On Sat, 11 Jul 2020, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-kfr.434.mcz
==================== Summary ====================
Name: Graphics-kfr.434 Author: kfr Time: 11 July 2020, 10:18:06.272175 am UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 Ancestors: Graphics-mt.433
I wanted to subclass Rectangle with a instance variable to carry some
state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
=============== Diff against Graphics-mt.433 ===============
Item was changed: ----- Method: Rectangle class>>encompassing: (in category 'instance
creation') -----
encompassing: listOfPoints "A number of callers of encompass: should use this method." | topLeft bottomRight | topLeft := bottomRight := nil. listOfPoints do: [:p | topLeft == nil ifTrue: [topLeft := bottomRight := p] ifFalse: [topLeft := topLeft min: p. bottomRight := bottomRight max:
p]].
^self origin: topLeft corner: bottomRight!
^ topLeft corner: bottomRight!
Item was changed: ----- Method: Rectangle class>>merging: (in category 'instance
creation') -----
merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | listOfRects do: [:r | minX ifNil: [minX := r topLeft x. minY := r
topLeft y.
maxX := r bottomRight x. maxY := r
bottomRight y]
ifNotNil: [minX := minX min: r topLeft x.
minY := minY min: r topLeft y.
maxX := maxX max: r bottomRight x.
maxY := maxY max: r bottomRight y]].
^ self origin:minX@minY corner: maxX@maxY!
^ minX@minY corner: maxX@maxY!
Hi,
In Rectangle>>merging: it is quite a bit slower to use points topLeft and bottomRight, than reading out the min and max of each side. Also looping with allButFirstDo: is slower than the original method.
This seems to be fastest:
Rectangle>>merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | minX := listOfRects first topLeft x. minY := listOfRects first topLeft y. maxX := listOfRects first bottomRight x. maxY := listOfRects first bottomRight y. listOfRects do: [:r | minX := minX min: r topLeft x. minY := minY min: r topLeft y. maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]. ^self origin: minX@minY corner: maxX@maxY
Best, Karl
On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi leves@caesar.elte.hu wrote:
Hi Karl,
While both changes look right, they modify the behavior of those methods when their argument is an empty list. Instead of raising an error, they'll return a Rectangle with both fields set to nil. If the argument is really a list - aka SequenceableCollection, I suggest using more specific methods instead of #do: to process the list in order to trigger the error as soon as possible. For example, use #first to get the first element and #allButFirstDo: to iterate over the rest. That would simplify the method as well by making the nil checks unnecessary.
Levente
On Sat, 11 Jul 2020, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-kfr.434.mcz
==================== Summary ====================
Name: Graphics-kfr.434 Author: kfr Time: 11 July 2020, 10:18:06.272175 am UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 Ancestors: Graphics-mt.433
I wanted to subclass Rectangle with a instance variable to carry some
state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
=============== Diff against Graphics-mt.433 ===============
Item was changed: ----- Method: Rectangle class>>encompassing: (in category 'instance
creation') -----
encompassing: listOfPoints "A number of callers of encompass: should use this method." | topLeft bottomRight | topLeft := bottomRight := nil. listOfPoints do: [:p | topLeft == nil ifTrue: [topLeft := bottomRight := p] ifFalse: [topLeft := topLeft min: p. bottomRight := bottomRight max:
p]].
^self origin: topLeft corner: bottomRight!
^ topLeft corner: bottomRight!
Item was changed: ----- Method: Rectangle class>>merging: (in category 'instance
creation') -----
merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | listOfRects do: [:r | minX ifNil: [minX := r topLeft x. minY := r
topLeft y.
maxX := r bottomRight x. maxY := r
bottomRight y]
ifNotNil: [minX := minX min: r topLeft x.
minY := minY min: r topLeft y.
maxX := maxX max: r bottomRight x.
maxY := maxY max: r bottomRight y]].
^ self origin:minX@minY corner: maxX@maxY!
^ minX@minY corner: maxX@maxY!
Hi Karl,
On Sun, 12 Jul 2020, karl ramberg wrote:
Hi,
In Rectangle>>merging: it is quite a bit slower to use points topLeft and bottomRight, than reading out the min and max of each side.
Indeed. Each iteration creates a new point if you use points.
Also looping with allButFirstDo: is slower than the original method.
I suppose your list of rectangles was an OrderedCollection when you measured performance because for most SequenceableCollections #allButFirstDo: is slightly faster than #do: by definition. OrderedCollection overrides #do: for better performance but does not have such implementation for #allButFirstDo:/#allButLastDo:.
Levente
This seems to be fastest:
Rectangle>>merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | minX := listOfRects first topLeft x. minY := listOfRects first topLeft y. maxX := listOfRects first bottomRight x. maxY := listOfRects first bottomRight y. listOfRects do: [:r | minX := minX min: r topLeft x. minY := minY min: r topLeft y. maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]. ^self origin: minX@minY corner: maxX@maxY
Best, Karl
On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi leves@caesar.elte.hu wrote: Hi Karl,
While both changes look right, they modify the behavior of those methods when their argument is an empty list. Instead of raising an error, they'll return a Rectangle with both fields set to nil. If the argument is really a list - aka SequenceableCollection, I suggest using more specific methods instead of #do: to process the list in order to trigger the error as soon as possible. For example, use #first to get the first element and #allButFirstDo: to iterate over the rest. That would simplify the method as well by making the nil checks unnecessary. Levente On Sat, 11 Jul 2020, commits@source.squeak.org wrote: > A new version of Graphics was added to project The Inbox: > http://source.squeak.org/inbox/Graphics-kfr.434.mcz > > ==================== Summary ==================== > > Name: Graphics-kfr.434 > Author: kfr > Time: 11 July 2020, 10:18:06.272175 am > UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 > Ancestors: Graphics-mt.433 > > I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle > > =============== Diff against Graphics-mt.433 =============== > > Item was changed: > ----- Method: Rectangle class>>encompassing: (in category 'instance creation') ----- > encompassing: listOfPoints > "A number of callers of encompass: should use this method." > | topLeft bottomRight | > topLeft := bottomRight := nil. > listOfPoints do: > [:p | topLeft == nil > ifTrue: [topLeft := bottomRight := p] > ifFalse: [topLeft := topLeft min: p. > bottomRight := bottomRight max: p]]. > + ^self origin: topLeft corner: bottomRight! > - ^ topLeft corner: bottomRight! > > Item was changed: > ----- Method: Rectangle class>>merging: (in category 'instance creation') ----- > merging: listOfRects > "A number of callers of merge: should use this method." > | minX minY maxX maxY | > listOfRects > do: [:r | minX > ifNil: [minX := r topLeft x. minY := r topLeft y. > maxX := r bottomRight x. maxY := r bottomRight y] > ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y. > maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]]. > + ^ self origin:minX@minY corner: maxX@maxY! > - ^ minX@minY corner: maxX@maxY!
Yes, I tested with OrderedCollection. Changed to Array and the speed was better. It seems most senders of the Rectangle>>merging: are with Array eg: subBnds := Rectangle merging: (submorphs collect: [:m | m fullBounds]). But DamageRecorder is an OrderedCollection for example.
I'm not sure how big impact speed will have here though. I can't measure speed difference for small collections with up to 5000 elements (which will probably cover most use cases)
Change set in attachment.
Best, Karl
On Sun, Jul 12, 2020 at 4:12 PM Levente Uzonyi leves@caesar.elte.hu wrote:
Hi Karl,
On Sun, 12 Jul 2020, karl ramberg wrote:
Hi,
In Rectangle>>merging: it is quite a bit slower to use points topLeft
and bottomRight, than reading out the min and max of each side.
Indeed. Each iteration creates a new point if you use points.
Also looping with allButFirstDo: is slower than the original method.
I suppose your list of rectangles was an OrderedCollection when you measured performance because for most SequenceableCollections #allButFirstDo: is slightly faster than #do: by definition. OrderedCollection overrides #do: for better performance but does not have such implementation for #allButFirstDo:/#allButLastDo:.
Levente
This seems to be fastest:
Rectangle>>merging: listOfRects "A number of callers of merge: should use this method." | minX minY maxX maxY | minX := listOfRects first topLeft x. minY := listOfRects first topLeft y. maxX := listOfRects first bottomRight x. maxY := listOfRects first bottomRight y. listOfRects do: [:r | minX := minX min: r topLeft x. minY := minY min: r topLeft y. maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]. ^self origin: minX@minY corner: maxX@maxY
Best, Karl
On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi leves@caesar.elte.hu
wrote:
Hi Karl, While both changes look right, they modify the behavior of those
methods
when their argument is an empty list. Instead of raising an error, they'll return a Rectangle with both
fields
set to nil. If the argument is really a list - aka SequenceableCollection, I
suggest
using more specific methods instead of #do: to process the list in
order
to trigger the error as soon as possible. For example, use #first to get the first element and
#allButFirstDo: to
iterate over the rest. That would simplify the method as well by
making
the nil checks unnecessary. Levente On Sat, 11 Jul 2020, commits@source.squeak.org wrote: > A new version of Graphics was added to project The Inbox: > http://source.squeak.org/inbox/Graphics-kfr.434.mcz > > ==================== Summary ==================== > > Name: Graphics-kfr.434 > Author: kfr > Time: 11 July 2020, 10:18:06.272175 am > UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84 > Ancestors: Graphics-mt.433 > > I wanted to subclass Rectangle with a instance variable to carry
some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new
subclass SuperRectangle > > =============== Diff against Graphics-mt.433 =============== > > Item was changed: > ----- Method: Rectangle class>>encompassing: (in category
'instance creation') -----
> encompassing: listOfPoints > "A number of callers of encompass: should use this method." > | topLeft bottomRight | > topLeft := bottomRight := nil. > listOfPoints do: > [:p | topLeft == nil > ifTrue: [topLeft := bottomRight := p] > ifFalse: [topLeft := topLeft min: p. > bottomRight := bottomRight
max: p]].
> + ^self origin: topLeft corner: bottomRight! > - ^ topLeft corner: bottomRight! > > Item was changed: > ----- Method: Rectangle class>>merging: (in category 'instance
creation') -----
> merging: listOfRects > "A number of callers of merge: should use this method." > | minX minY maxX maxY | > listOfRects > do: [:r | minX > ifNil: [minX := r topLeft x. minY
:= r topLeft y.
> maxX := r bottomRight x.
maxY := r bottomRight y]
> ifNotNil: [minX := minX min: r
topLeft x. minY := minY min: r topLeft y.
> maxX := maxX max: r
bottomRight x. maxY := maxY max: r bottomRight y]].
> + ^ self origin:minX@minY corner: maxX@maxY! > - ^ minX@minY corner: maxX@maxY!
squeak-dev@lists.squeakfoundation.org