41119 - nsFrameManager destructor is slow, maybe avoid frame destruction notification?

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[buster]

FrameManager owns the frame tree.
When FrameManager is destroyed, it destroys the frame tree.
As each frame is destroyed, it notifies the FrameManager about it's destruction.  
Is this really necessary?  The only reason I can see for doing this is to free 
up any properties stored on behalf of frame in a hash table owned by the 
FrameManager.   Maybe there's a more efficient way of doing this, since few 
frames store properties, and few properties actually need destructors called 
(some are just basic types like ints, some are pointers to other frames like 
Waterson's secondary frame pointers.)

This will effect page transition time.

Thoughts?

Comment 1

[Chris Waterson]

A frame still needs to notify the frame manager when its destroyed; e.g., by a
resize reflow or some DOM operation.

Maybe the optimization is to add a "fast path" destroy method that accepts a
"notify frame manager" argument? E.g.,

  class nsFrame {
  public:
    Destroy(nsIPresContext* aPresContext, PRBool aNotify = PR_TRUE);
  };

Only the frame manager would ever call Destroy(presContext, PR_FALSE);

Alternatively, you could try unhooking the pres content from the pres shell
before destroying the frame tree: nsFrame::Destroy() tries to get the shell, and
will only notify the frame manager if the pres context *has* a shell. Not sure
what other effects this would have...

Comment 2

[buster]

This bug has been marked "future" because we have determined that it is not 
critical for netscape6.0. 
If you feel this is an error, or if it blocks your work in some way -- please 
attach your concern to the bug for reconsideration.

We may reopen this bug during the performance improvement phase of the project, 
if the frame destructor turns out to be a bottleneck for page transition.

assigning to waterson so it's on his bug list.

Comment 3

[Brian Ryner (not reading)]

I can take this.

Comment 4

[Brian Ryner (not reading)]

Don't we already free up the properties by calling DestroyPropertyList() from
nsFrameManager::Destroy() ?

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

I still need to do perf testing with this, but I can't do that very well at
home.

Comment 6

[Brian Ryner (not reading)]

Attachment: patch v2.0

This also avoids flushing reflows for the destroyed frames.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch v2.1

Let's try this again, without including part of the outliner content model
patch.

Comment 8

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

Might you need to make nsPresShell::Destroy call CancelAllReflowCommands to
avoid leaking reflow commands?

Comment 9

[Brian Ryner (not reading)]

Attachment: patch v2.2

make sure to call CancelAllReflowCommands()

Comment 10

[Brian Ryner (not reading)]

dbaron, can you review the new patch?

Comment 11

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

Comment on attachment 59406 [details] [diff] [review]
patch v2.2

r=dbaron, except you don't need the member initializer and you can remove all
the other member initializers since nsPresShell uses a
ZEROING_OPERATOR_NEW.

Comment 12

[Chris Waterson]

AFAICT, this change seems to be mandating that frame properties _cannot_ rely on
being finalized when the pres shell is destroyed. This would imply that 1) any
dynamic allocation for a property must happen from the pres shell's arena, and
2) properties _must not_ hold owning references to objects outside the pres shell.

I think that this added constraint is reasonable, but you'll need to look at the
clients of nsIFrameManager::SetFrameProperty to verify that they all play by the
rules. For example, nsBlockBandData::StoreMaxElementSize will leak an nsSize
object, no?

Or am I confused?

Comment 13

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

nsFrameManager::Destroy calls DestroyPropertyList.  Isn't that sufficient?

Comment 14

[Chris Waterson]

Okay, missed that path. sr=waterson

Comment 15

[Brian Ryner (not reading)]

checked in