[squeak-dev] duplicated rectangles in DamageRecorder

  Hello,

  I noticed that I often get duplicated (i.e. equal) rectangles in
invalidRects of DamageRecorder.

  In #recordInvalidRect: you make the part to read:

        invalidRects do:
                [:rect |
                       (newRect origin = rect origin and: [newRect corner = rect corner])
                        ifTrue: [Display reverse: newRect].
                       ...

and you'll see when it happens.

  If I understand it correctly the duplicate there doesn't result in
multiple drawing (via the 'validList' logic), but I do see a bit of
improvement in browser opening benchmarks etc. (2% or such, could be
just jitter, though), if I change "Display reverse: newRect" to "^
self".

  I'd like to hear if this makes any sense, or more consistent
benchmark results, etc.

-- Yoshiki

Hi Yoshiki,

Yoshiki Ohshima wrote:
Playing a bit with it (in Cuis), it seems to happen because of unneeded
calls to #changed. I could avoid some of them by making property setter
methods do nothing if the property is set. For example,
BorderedMorph>>#borderWidth: TextMorph>>#backgroundColor: and
TransformMorph>>#offset can use a check with the argument and just exit,
like for example Morph>>#extent:. Besides,
MenuMorph>>#popUpAt:forHand:in:allowKeyboard: does not need the last
line (self changed).

In any case, unneeded calls to self changed are usual, as it says in the
last comment of TextMorph>>#fit. So, I think that the check and early
exit you propose in #recordInvalidRect: is a good idea and will include
it in Cuis.

Cheers,
Juan Vuletich

On Friday 02 Oct 2009 5:23:26 pm Juan Vuletich wrote:
> In any case, unneeded calls to self changed are usual, as it says in the
> last comment of TextMorph>>#fit. So, I think that the check and early
> exit you propose in #recordInvalidRect: is a good idea and will include
> it in Cuis.
Why not fix recordInvalidRect: to drop duplicates? Duplicates are getting in
because of the test for large intersections before a merge. This will also
avoid triggering Merge threshold unnecessarily.

Subbu

Hi Subbu,

K. K. Subramaniam wrote:
That's exactly what Yoshiki suggested and I said I'm doing in Cuis,
isn't it?

Cheers,
Juan Vuletich

On Friday 02 Oct 2009 6:07:31 pm Juan Vuletich wrote: Yes, indeed. I deserve a whack on my head for rushing a response before
reading his mail completely.

Subbu

At Fri, 02 Oct 2009 08:53:26 -0300,
Juan Vuletich wrote:
>
> Playing a bit with it (in Cuis), it seems to happen because of unneeded
> calls to #changed. I could avoid some of them by making property setter
> methods do nothing if the property is set. For example,
> BorderedMorph>>#borderWidth: TextMorph>>#backgroundColor: and
> TransformMorph>>#offset can use a check with the argument and just exit,
> like for example Morph>>#extent:.

  Ah ha.  Good catch.

> Besides,
> MenuMorph>>#popUpAt:forHand:in:allowKeyboard: does not need the last
> line (self changed).

  Right.  fillStyle: for example does it.

> In any case, unneeded calls to self changed are usual, as it says in the
> last comment of TextMorph>>#fit. So, I think that the check and early
> exit you propose in #recordInvalidRect: is a good idea and will include
> it in Cuis.

  Thank you for validating the idea!

-- Yoshiki

2009/10/3 Yoshiki Ohshima <[hidden email]>:
I think that catching excessive #changed sends is like hunting for a ghosts :)
There should be a point, which could check that invalidated rectangle
is already in damage recorder.


> -- Yoshiki
>
>



--
Best regards,
Igor Stasenko AKA sig.

Igor Stasenko schrieb:
>
> I think that catching excessive #changed sends is like hunting for a ghosts :)
> There should be a point, which could check that invalidated rectangle
> is already in damage recorder.
>  
Exactly. I have a patch (at work) which optimizes damage recording quite
a bit (specifically, it avoids excessive growing of the damage region
when two rectangles are merged). As a side effect, it makes sure that
the final list of damage rectangles is non-overlapping.
I will post it on Monday when I've picked it up from that image at work.

Cheers,
Hans-Martin

There is a bug in layout that make it necessary to add 'self changed' at various places where it should not be.
Like in this example:
http://tracker.immuexa.com/browse/SQ-132
This server is down right now :-(

Karl