I'm looking at DHTML performance in bug 124178 and one big time offender is CreateRegion called from nsContainerFrame::SyncFrameViewAfterReflow. 29% of the total time is spent in that function. 52% is a call to nsComponentManager::CreateInstance (cache this?) 48% is nsRegionWin::Init (don't know if anything can be done about this) Assuming the CreateInstance can be removed that's 15% of the total reflow time in bug 124178. It can be that the problem is that CreateRegion is called too often but it looks like an easy kill to cache whatever component is created. (OS: Windows 2000)

This has probably nothing do with views after all. Should I move it to layout?

Uh. Just looked at the code. So Regions are XPCOM components? Uh. We may create thousands of these during a page layout and DHTML animations. (In this case I have 278413 calls to CreateRegion). This is just so wrong. We very much don't need the XPCOM overhead for these objects. Figures: In bug 124178 CreateRegion is 30% of the total time and of that time 17% (5% total) is just finding the factory.

DeXPCOMifying regions, or even better, caching them in some global cache, would be a very good thing.

If the CreateInstance() call can't be eliminated, we could always lookup the factory and cache that, this way the component manager hashes n' all that are take out of the equation. Alternatively, depending on the region lifetime patterns, a little local cache of regions that are reused in stead of always recreating the regions might make sense.

I say we cache regions. Creating a region could be expensive on some platforms.

Doing that *and* caching the factory is probably the best option here, cheak enough to do...

Even with caching of the factory, there would be the addref/release stuff. Is that really needed for regions? Can we not have a simple ownership model? I guess we might need some kind of factory to produce different region types on different platforms or can that be done with build magic?

True, but addref/release doesn't really cost anything unless they're heavily used, so the fact that these objects are refcounted doesn't necessarily mean worse performance.

I tried caching a factory but that don't win us much. 5-10% of the creation time. What really would be a good win is the cache. I could probably live with us having the XPCOM shell around the regions if they were cached. Any ideas for a design? Is there any other objects that already do this?

This is the patch I tested with. There are other places that also creates regions but this was the place that create a lot (hundreds of thousands).

If the call in nsContainerFrame::SyncViewAfterReflow is the vast majority of the region creation problem, I can add a new interface to the view manager that takes a rect instead of a region, avoiding the need to create a region at all.

Actually, the best thing to do here is probably just to cache one region in nsContainerFrame and use it every time instead of calling CreateRegion.

It feels much easier to just use a rect if that's possible. I'm sure noone accepts a new member variable on the container frame and other caches are difficult (I think) because we don't want to hold on to a region longer than necessary.

I think it's OK to hold onto one region forever.

Bulk moving Mozilla1.1 bugs to future-P2. I will pull from this list when scheduling post Mozilla1.0 work.

I looked at how the nsIRegion is used and as far as I can tell they could just as well be normal non-XPCOM objects. There's nothing fance about them and theu don't have a long life span. I've tried to make them such but are running into some build/link issues. Right now we have one XP lib (gkgfx.dll) and one platform lib (gfxwin.dll). Nothing in gfxwin.dll is exported except for the XPCOM symbols but I need a way to call the nsRegionWin constructor some way. My first thought was to have a generic factory method in gkgfx.dll that called the native one in gfxwin.dll but that would add a dependancy on the platform part from the XP part. Another idea I have is to do like nsFileSpec, have the same class name and interface for all platforms but different implementations but then gkgfx.dll would suddenly contain platform dependant code. Why do we need this division anyway? Can we not do with one bug gfx.dll that contain everything needed?

The platform gfx library is a component, whereas the XP one must be linked against by other libraries (which we generally shouldn't do for components).

So are you saying that it's not possible to solve without breaking the design or that it should be put in gkgfx.dll?

> Another idea I have is to do like nsFileSpec, have the same class name and > interface for all platforms but different implementations but then gkgfx.dll > would suddenly contain platform dependant code. This approach seems like it could work. What about mkaply's XP region code, though?

So that was what it was! I found an nsRegion class that looked unused. As far as I can tell, there is really no need for platform specific regions, except maybe because they can be optimized further. nsRegionWin uses the OS region which might be faster than a hand made region. I will try doing that switch. Is nsRegion stable? Used for OS/2?

I think in a few places we want access to the native region wrapped by our region object, but I could be wrong. Other than that I'd be happy to use the XP region code.

This is some work in progress that I did before the summer (and tuned a little yesterday). It works on Windows. What it does is to convert nsIRegion to a base class for concrete classes of which nsRegionWin is the used. The compont code is removed and the xpcom factory too. Instead there is a method that will have to be different for each platform, unless we want to skip native regions altogether and go with nsRegionImpl. I don't think we want that since I expect the OS regions to be quite optimized. Anyway, it should be easy to fix. The nsRegion usage is quite simple except maybe for the nsScriptableRegion which I'm note 100% sure I got right. Normally it's just a region created and then deallocated in the same method. One argument for the XP nsRegionImpl then is that it would be possible to create the regions on the stack. I've kept the nsIRegion name (including the I) to avoid changing everything everywhere. So, what do you think? The correct approach?

Missed this new file.

Sorry Daniel, I should have mentioned this earlier, but I have just posted a patch to bug 164625 which, as a side effect, completely solves this problem on all platforms :-).

Well, by "completely" I mean "completely fixed for nsContainerFrame::SyncFrameViewAfterReflow". We create regions in other places, but not nearly at such a large rate. Other places==1 for each nsView::mDirtyRgn and 1 for each NS_PAINT event handled, AFAIK. The NS_PAINT one can go away once we switch to using the region in the paint event instead of creating a region from the paint rect. The nsView::mDirtyRgn ones probably should be converted to nsRegion at some point for speed and size improvement.

Cool. Then I find something else to do. Hmm, the sun's still shining... :-)

Linkoeping is quite tolerable in the summer. :)

I checked in the fix for 164625, so I'm marking this FIXED too.