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 at 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 at minY corner: maxX at maxY
> >
> > Best,
> > Karl
> >
> > On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi <leves at 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 at 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 at minY corner: maxX at maxY!
> >       > -     ^ minX at minY corner: maxX at maxY!
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20200712/213e18ff/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RectangleMergeTesting.1.cs
Type: application/octet-stream
Size: 2688 bytes
Desc: not available
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20200712/213e18ff/attachment-0001.obj>