249810 - Reduce allocations by nsViewManager

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[jim_nance]

I ran some jprofs and noticed a lot of time spent in malloc/free due to calls
from various places in nsViewManager code.  I added some print statements to see
what was going on, and it seem that the code to build the ZTree is called every
time a mouse movement event is seen.  The trees are not large, but we get a lot
of events.  I have two patches that reduce the number of allocations.  The first
keeps a per-manager free list of ZTree nodes.  The second allocates all ZTree
and Element nodes from an Arena, which is destroyed after the events are used. 
I prefer the second patch, but it is more intrusive, so I am including both of
them here.

Comment 1

[jim_nance]

Attachment: Per-manager free list for ZTree nodes

This patch implements a per-manager free list for zTree elments.  It
essentially eleminates the allocations for zTree nodes, but it does not address
the allocation of DisplayElement nodes.

Comment 2

[jim_nance]

Attachment: Arena based tree allocation

This patch allocates all ZTree and DisplayElement nodes from an Arena.	The
Arena is destroyed after the DisplayElements are used.

Comment 3

[jim_nance]

I would also appreciate knowing who I should ask for r/sr of these patches.  I
dont know who ownes this code.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I own it.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // delete aNode->mDisplayElement;
+    // delete aNode;

Add a comment explaining about the arena.

I'd also appreciate a comment somewhere explaining what the overall strategy is
--- "allocate the display list elements and ZTree nodes in an arena associated
with the display list".

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 152309 [details] [diff] [review]
Arena based tree allocation

r+sr with those comments addressed. Thanks very much!

Comment 7

[jim_nance]

Wow!  Thats the fastest review I have ever gotten.  I checked the code in with
the additional comments you asked for.  It looks like it might have shaved a
couple ms off the Tp tests.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

only 88 bytes in size increase. that's pretty impressive.

Comment 9

[jim_nance]

Attachment: Add calls to PL_FreeArenaPool

I found that things run faster if I add calls to PL_FreeArenaPool before I
destroy the Arenas.  This call adds the memory associated with the arena to a
global freelist which the arena subsystem maintains.  This allows code which
creates and destroys a lot of arenas (which nsViewManager does) to avoid having
to go to malloc for the first block.

Comment 10

[jim_nance]

Before:
                  7 nsViewManager::AddToDisplayList(nsView*, DisplayZTreeNode*&
                  4 nsFixedSizeAllocator::Alloc(unsigned int)
                  2 nsLineLayout::NewPerSpanData(nsLineLayout::PerSpanData**)
                  1 FrameArena::AllocateFrame(unsigned int)
                  1 nsLineLayout::NewPerFrameData(nsLineLayout::PerFrameData**)
                  1 nsFixedSizeAllocator::Init(char const*, unsigned int const*
                  1 nsFixedSizeAllocator::AddBucket(unsigned int)
 47193   3       17 PL_ArenaAllocate
                  9 PR_Malloc
                  2 UnlockArena
                  2 LockArena
                  1 PR_Lock

After:
                  5 nsFixedSizeAllocator::Alloc(unsigned int)
                  1 nsViewManager::AddToDisplayList(nsView*, DisplayZTreeNode*&
                  1 FrameArena::AllocateFrame(unsigned int)
 47193   0        7 PL_ArenaAllocate
                  5 PR_Malloc
                  1 UnlockArena
                  1 LockArena

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Well, this bug _was_ fixed, for 7 minutes, until it was subsequently backed out
(it was referenced with a different, non-related bug #, so it was fairly hard to
track).

2004-07-08 20:39 	jim_nance%yahoo.com 	/ cvsroot/ mozilla/ view/ src/
nsViewManager.cpp 	3.348 	  	0/4  	Backing out last change. I didn't realize the
tree was frozen

2004-07-08 20:32 	jim_nance%yahoo.com 	/ cvsroot/ mozilla/ view/ src/
nsViewManager.cpp 	3.347 	  	4/0  	Fix bug 152596 - Add calls to
PR_FreeArenaPool() before PR_FinishArenaPool()
so that memory is added to the global free lists. r+sr=roc

The original patch http://bugzilla.mozilla.org/attachment.cgi?id=152309&action=view

landed:

3.343	jim_nance%yahoo.com	2004-07-05 06:10	 	Fix bug 249810 - Reduce allocations
by nsViewManager. r+sr = roc

So http://bugzilla.mozilla.org/attachment.cgi?id=152596&action=view still hasn't
landed.

Reopening, for now.

Comment 12

[jim_nance]

Checked in the last patch.  Hopefully with the right bug number this time :-)