Closed
Bug 98828
Opened 23 years ago
Closed 23 years ago
43% of PresShell::ProcessReflowCommand() spent in new and delete
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla0.9.5
People
(Reporter: jst, Assigned: jst)
References
()
Details
(Keywords: perf, Whiteboard: [HAVE FIX])
Attachments
(2 files)
8.59 KB,
patch
|
Details | Diff | Splinter Review | |
8.20 KB,
patch
|
dbaron
:
review+
waterson
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Comment 2•23 years ago
|
||
r=? sr=?
Assignee | ||
Comment 3•23 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. :-)
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 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.
Assignee | ||
Updated•23 years ago
|
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)
Attachment #48677 -
Flags: review+
Comment 9•23 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?
Assignee | ||
Comment 10•23 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.
Comment 11•23 years ago
|
||
cc'ing scc for C++ expert opinion.
/be
Comment 12•23 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.
Assignee | ||
Comment 13•23 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•23 years ago
|
||
Comment on attachment 48677 [details] [diff] [review]
Make gcc happy, same as above
sr=waterson
Attachment #48677 -
Flags: superreview+
Assignee | ||
Comment 15•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•