Letting the JS on above URL run for a while in quantify shows that we spend a whopping 43% of the time spent in PresShell::ProcessReflowCommand() just allocating and deallocating nsSpaceManager objects (24.6%/18.6%), mostly from nsBlockFrame::Reflow(). I went ahead and added a little cache that is caches 4 nsSpaceManager objects which lets us re-use these objects in stead of always creating new ones just to delete them shortly after. Patch coming up. Even with my fix we're still an order of magnitude slower than IE 5.5 using this testcase (as if that's something we didn't know ;-) Thanks to heikki for pointing me at this testcase.

r=? sr=?

Ok, so gcc has issues with my fancy constructor and destructor calls in the patch :-) I'll see what I can do about that...

Seems reasonable in the short term, although in the long term I'll want to rip this out when fixing bug 90725 and then bug 86950 (which will require storing the few *needed* space managers as frame properties). I think it's preferable to use placement new rather than an explicit call to the constructor, and I'm not sure whether all compilers will like your destructor call syntax: |this->~nsSpaceManager()| might be preferable -- it's what seems to be used in other places in the tree. Also, why not just use |NS_IMPL_QUERYINTERFACE1| rather than writing out the map explicitly? Other than that, r=dbaron.

And I wrote that before seeing your comment about gcc. :-)

David, thanks for the feedback, you're right, placemente new works where as my direct constructor call doesn't. Interestingly enough, |nsSpaceManager::~nsSpaceManager()| does the right thing in VC++, so does |this->nsSpaceManager::~nsSpaceManager()| (which also works in gcc), but |this->~nsSpaceManager()| does *not* call the destructor in VC++ (it does compile, however), at least my breakpoints in the destructor were not called. Weird. Anyway, the new patch works on windows and linux.

Comment on attachment 48677 [details] [diff] [review] Make gcc happy, same as above r=dbaron (although I wouldn't mind seeing the QI implementing using NS_IMPL_QUERYINTERFACE1 instead)

> |this->nsSpaceManager::~nsSpaceManager()| (which also works in gcc), but > |this->~nsSpaceManager()| does *not* call the destructor in VC++ (it does Shortly after reading this, I noticed that the pattern |this->~foo()| is used *everywhere* in nsStyleStruct.h. Does VC++ really not handle this correctly? Do these uses need to be cleaned up?

David, using NS_IMPL_QUERYINTERFACE1 here won't happen because a) I hate those macros :-), and b) if I use that macro (which I won't do anyway :-) there's no way to hook into the last release call (i.e. NS_IMPL_RELEASE_WITH_DESTROY). Thanks for the review. Tingley, I could be wrong about the behavior of |this->~nsSpaceManager()| in VC++, I'd haveto re-check that again to be sure, I might have run into build issues or somesuch so I wasn't running the code I thought I was, or something.

cc'ing scc for C++ expert opinion. /be

this->~nsFoo() works fine in VC++. I used that pattern in the style system stuff, and have set breakpoints inside the destructor and watched them get hit when I was ensuring my code didn't leak.

Thanks hyatt, I must have been dreaming or something when I saw it not working (or I wasn't executing the code I thourhg I was...).

Comment on attachment 48677 [details] [diff] [review] Make gcc happy, same as above sr=waterson

Fix checked in.