234250 - nsRenderingContextGTK makes too many clip regions

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[jim_nance]

After seeing nsRegionGTK::SetTo() show up on a jprof profile, I started looking
at its callers.  It seems that nsRenderingContextGTK::SetClipRegion() calls this
method twice when it frequently only needs to call it once.  The code path where
this happens is this.  If mClipRegion is NULL, CreateClipRegion() creates a new
region and calls SetTo() to set the region to the size of mSurface.  However, if
SetClipRegion() was called with aCombine==nsClipCombine_kReplace (and it almost
always is) then we just call SetTo() again.  So the first one was unnecessary.

The attached patch adds an argument to SetClipRegion() to control whether the
region get SetTo() called on it or not.  Beyond calling SetTo() unnecessarily, I
think the code may have some logic errors, but I do not know enough about what
it is susposed to be doing to fix them yet.  I would appreciate some comment on
these issues:

1) SetClipRegion checks to see if the creation of mClipRegion failed, but its
callers always assume it worked.  I assume that the callers should check, but
what should they do if it failed?

2) SetClipRegion only calls SetTo() if it creates a new clip region.  If a
region already exists, it does not alter it.  Is that really what is susposed to
happen?

Comment 1

[jim_nance]

Attachment: Patch to reduce SetTo() calls

Comment 2

[jim_nance]

Roland,  You asked me to make my changes in the xlib code as well as the gtk
code.  I looked at the xlib code a few minutes ago, and the changes are not
really applicapable as there is no CreateClipRegion() function over there.  I
looked in CVS and CreateClipRegion was added to the gtk code in 2000, so it
seems that these two code streams diverged some time ago.  Is this unexpected?

Comment 3

[Roland Mainz]

[e-mail] wrote:
> Roland,  You asked me to make my changes in the xlib code as well as the gtk
> code.  I looked at the xlib code a few minutes ago, and the changes are not
> really applicapable as there is no CreateClipRegion() function over there.  I
> looked in CVS and CreateClipRegion was added to the gtk code in 2000, so it
> seems that these two code streams diverged some time ago.  Is this unexpected?

Not really, I forgot that the clipping code in Xlib is slightly different.
Search for |new nsRegionXlib()| ... the Xlib code avoided going througth
|do_CreateInstance()| because this ends-up in zillions of completely unneccesary
method calls.

Comment 4

[jim_nance]

Should the gtk code do the same thing?

Comment 5

[Roland Mainz]

[e-mail] wrote:
> Should the gtk code do the same thing?

I don't see a reason why not. The whole idea of using the component manager for
gfx implementation specific objects is useless anyway since objects are only
compatible to objects from the same device, e.g. you cannot mix objects for
gfx/src/ps with objects from gfx/src/gtk (and you would not be able to mix
objects if they target different output devices (two video cards or video and
printer output, however until now only gfx/src/xlib and gfx/src/windows can do
that)) - that will cause a crash sooner or later.
For nsRenderingContext* objects I already fixed that via adding a new method to
nsIDeviceContext which creates nsIRenderingContext objects for _exactly_ that
nsIDeviceContext object instance.

Comment 6

[Christopher Blizzard (:blizzard)]

Comment on attachment 141369 [details] [diff] [review]
Patch to reduce SetTo() calls

Clearing flags until confusion is cleared up.

Comment 7

[jim_nance]

Roland, would you port your xlib changes over to gtk?  It is a bit beyond me.

Comment 8

[jim_nance]

Attachment: Removes do_CreateInstance calls

Implements Roland's suggestion to create clip regions w/o calling
do_CreateInstance().

Comment 9

[jim_nance]

Roland, this turned out to be simplier than I thought.  What do you think of the
attached patch.  It replaces calls to do_CreateInstance() with 'new nsRegionGTK',
similar to how you did it in the Xlib code.  I also cleaned up some duplicated
code, so the end product is a little smaller as well.

This does not implement the functionality of the original patch, which was to
reduce the number of SetTo() calls.

Comment 10

[jim_nance]

ping ?

Comment 11

[Roland Mainz]

[e-mail] wrote:
> ping ?

pong!
Somehow I didn't received the bugzilla emails for comment #9.

attachment 151613 [details] [diff] [review] looks OK but I do not like the name |CreatePrivateClipRegion|
... it's still the same what |CreateClipRegion| does... can you change that,
please ?

Comment 12

[Roland Mainz]

Reassign bug to Jim...

Comment 13

[jim_nance]

Attachment: Patch incorporating Roland's requests

Here is the patch with the changes you requested incorporated.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 153785 [details] [diff] [review]
Patch incorporating Roland's requests

I don't like the name CreateClipRegion so much ... sometimes it doesn't do
anything. I'd prefer, say, "EnsurePrivateClipRegion". Also, setting reviewer to
caillon in hopes of a speedier review.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 153785 [details] [diff] [review]
Patch incorporating Roland's requests

Patch looks fine.  I do however have a few nits I'd like fixed before this gets
checked in.  It also looks as though we have a theoretical crash if creating a
region fails due to allocation failure, since all of the callers assume this
never fails.  That is not your fault, and quite unlikely, so I won't hold you
to fix that.  Maybe file a bug on it, though for robustness' sake, and also to
make timeless happy.

>+void
>+nsRenderingContextGTK::CreateClipRegion()
>+{
>+  // We have 3 cases to deal with:
>+  //  1 - There is no mClipRegion -> Create one
>+  //  2 - There is an mClipRegion shared w/ stack -> Duplicate and unshare
>+  //  3 - There is an mClipRegion and its not shared -> return
>+
>+  if(mClipRegion) {

Space between the if and paren.

>+    PRUint32 cnt = mStateCache.Count();
>+
>+    if (cnt > 0) {
>+      nsGraphicsState *state;
>+      state = (nsGraphicsState *)mStateCache.ElementAt(cnt - 1);
>+
>+      if (state->mClipRegion == mClipRegion) {
>+        mClipRegion = new nsRegionGTK;
>+        if(mClipRegion) {

Space between the if and paren.

>+          mClipRegion->SetTo(*state->mClipRegion);
>+        }
>+      }
>+    }
>+  } else {
>+
>+    PRUint32 w, h;
>+    mSurface->GetSize(&w, &h);
>+
>+    mClipRegion = new nsRegionGTK;
>+    if (mClipRegion) {
>+      mClipRegion->Init();
>+      mClipRegion->SetTo(0,0,w,h);

Spaces after the commas, please.

>+    }
>+  }
>+}
>+
>@@ -647,18 +645,18 @@ NS_IMETHODIMP nsRenderingContextGTK::Get
>   if (!aRegion || !mClipRegion)
>     return NS_ERROR_NULL_POINTER;
> 
>   if (mClipRegion) {
>     if (*aRegion) { // copy it, they should be using CopyClipRegion 
>       (*aRegion)->SetTo(*mClipRegion);
>       rv = NS_OK;
>     } else {
>-      nsCOMPtr<nsIRegion> newRegion = do_CreateInstance(kRegionCID, &rv);
>-      if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsIRegion> newRegion = new nsRegionGTK;
>+      if(newRegion) {

Space between the if and paren.


>         newRegion->Init();
>         newRegion->SetTo(*mClipRegion);
>         NS_ADDREF(*aRegion = newRegion);
>       }
>     }
>   } else {
> #ifdef DEBUG
>     printf("null clip region, can't make a valid copy\n");



r=caillon with the nits addressed.

Comment 16

[jim_nance]

Attachment: Patch incorporating Caillon's requests

Made changes requested by Caillon.  Will check in once I get it rebuilt and
smoketested.