98828 - 43% of PresShell::ProcessReflowCommand() spent in new and delete

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johnny Stenback (:jst)]

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.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Cut new/delete time from 43% to <0.2%

Comment 2

[Johnny Stenback (:jst)]

r=? sr=?

Comment 3

[Johnny Stenback (:jst)]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Johnny Stenback (:jst)]

Attachment: Make gcc happy, same as above

Comment 7

[Johnny Stenback (:jst)]

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 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 9

[Chase Tingley]

> |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?

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Brendan Eich [:brendan]]

cc'ing scc for C++ expert opinion.

/be

Comment 12

[David Hyatt]

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.

Comment 13

[Johnny Stenback (:jst)]

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 14

[Chris Waterson]

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

sr=waterson

Comment 15

[Johnny Stenback (:jst)]

Fix checked in.