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

RESOLVED FIXED in mozilla0.9.5



17 years ago
14 years ago


(Reporter: jst, Assigned: jst)


(Blocks: 1 bug, {perf})


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [HAVE FIX], URL)


(2 attachments)



17 years ago
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

17 years ago
Created attachment 48675 [details] [diff] [review]
Cut new/delete time from 43% to <0.2%

Comment 2

17 years ago
r=? sr=?

Comment 3

17 years ago
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. :-)

Comment 6

17 years ago
Created attachment 48677 [details] [diff] [review]
Make gcc happy, same as above

Comment 7

17 years ago
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.


17 years ago
Keywords: perf
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.5
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
Attachment #48677 - Flags: review+

Comment 9

17 years ago
> |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

17 years ago
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.


Comment 12

17 years ago
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

17 years ago
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

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

Attachment #48677 - Flags: superreview+

Comment 15

17 years ago
Fix checked in.
Last Resolved: 17 years ago
Resolution: --- → FIXED


17 years ago
Blocks: 21762


14 years ago
No longer blocks: 21762
Blocks: 21762
You need to log in before you can comment on or make changes to this bug.