BrowserChild::RecvDestroy can potentially run very long
Categories
(Core :: DOM: Content Processes, enhancement)
Tracking
()
People
(Reporter: jstutte, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
It seems that various hangs with just NotifiedImpendingShutdown show that we are processing a BrowserChild::RecvDestroy message on the child's main thread.
BrowserChild::RecvDestroy does various long-running things synchronously, like mozilla::ServoStyleSet::UpdateStylist() or mozilla::dom::ScriptLoader::Destroy() that seem to be mostly just complicated ways to free memory.
The starting point in the parent seems to be BrowserParent::Destroy, where we first send the Destroy to the browser child and then in case it was the last browser for this content process start the shutdown for this process.
There might be room for enhancement here in case the parent process knows it is shutting down itself and thus we could just shutdown all known content processes without all the ceremony of BrowserChild::RecvDestroy. If we really need this ceremony, we could anticipate at least the sending of the NotifyImpendingShutdown to happen before we do BrowserParent::Destroy and optimize some of the code paths in the content process.
The same optimization could take place when we know we are the last BrowserParent for this content process.
|
Reporter | |
Comment 1•2 years ago
|
||
If we want to maintain the current logic, a possible optimization inside BrowserChild::RecvDestroy could be:
- Ensure we get
NotifiedImpendingShutdownbefore we call (the last)BrowserChild::RecvDestroy - In case we are
ProcessChild::ExpectingShutdown():- Identify objects that need not be freed immediately (separating other shutdown logic from the memory free)
- Add those objects to a relatively late XPCOM shutdown phase with
ClearOnShutdownsuch that in release builds they will not be freed at all but will not cause leaks
|
|
Reporter | |
Comment 2•1 year ago
•
|
||
It seems that nsDocShell::Destroy is pretty common on these stacks.
Among those, ServoStyleSet::ShellDetachedFromDocument has a decent percentage.
Am I right in assuming that this function is only freeing memory? Could we just detach the ServoStyleSet object from the PresShell and post an event to the main thread to free the memory asynchronously?
And if(ProcessChild::ExpectingShutdown() we could even do something with ClearOnShutdown here to do it even later.
|
||
Comment 3•1 year ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #2)
Am I right in assuming that this function is only freeing memory?
For the most part, it also updates the DOM state etc.
Could we just detach the
ServoStyleSetobject from thePresShelland post an event to the main thread to free the memory asynchronously?
Yes, though not trivially because it calls into code that needs to otherwise run sync from e.g. DOM node removal... Is the slow part freeing the memory? Or going through the DOM cleaning up styles? Or something else?
|
|
Reporter | |
Comment 4•1 year ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
(In reply to Jens Stutte [:jstutte] from comment #2)
Could we just detach the
ServoStyleSetobject from thePresShelland post an event to the main thread to free the memory asynchronously?Yes, though not trivially because it calls into code that needs to otherwise run sync from e.g. DOM node removal... Is the slow part freeing the memory? Or going through the DOM cleaning up styles? Or something else?
That I do not know from the stack trace. It might be worth to set up some test situation with a large DOM tree and run a shutdown with profiling.
|
|
Reporter | |
Comment 5•4 months ago
•
|
||
We revisited content process shutdown in bug 1728331 and it seems that the changes from bug 1809134 are not really needed.
This means we cannot rely on ProcessChild::ExpectingShutdown() having been set before RecvDestroy runs, but its result can change at any time in parallel, so periodically checking it would still work to abort long running tasks, if we think this helps with anything.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
(In reply to Jens Stutte [:jstutte] from comment #2)
Am I right in assuming that this function is only freeing memory?
For the most part, it also updates the DOM state etc.
Could we just detach the
ServoStyleSetobject from thePresShelland post an event to the main thread to free the memory asynchronously?Yes, though not trivially because it calls into code that needs to otherwise run sync from e.g. DOM node removal... Is the slow part freeing the memory? Or going through the DOM cleaning up styles? Or something else?
I think the best course of action remains to understand, what actually causes the load here. There might be some potential for improvements that could even affect also normal operations, especially if there is some hidden quadratic runtime caused by some nesting hitting us only for large trees or similar.
|
|
Reporter | |
Comment 6•4 months ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #4)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
(In reply to Jens Stutte [:jstutte] from comment #2)
Could we just detach the
ServoStyleSetobject from thePresShelland post an event to the main thread to free the memory asynchronously?Yes, though not trivially because it calls into code that needs to otherwise run sync from e.g. DOM node removal... Is the slow part freeing the memory? Or going through the DOM cleaning up styles? Or something else?
That I do not know from the stack trace. It might be worth to set up some test situation with a large DOM tree and run a shutdown with profiling.
FWIW, I took a profile while closing a few tabs containing large things, most significantly probably searchfox with IndexedDB's ActorParent.cpp loaded (where I zoomed in). Obviously this is just one trivial case wit just a few element types in the tree, I assume, but might indicate where there could be some interesting loops.
It seems that bug 1896875 already did some changes to nsLineBox::DeleteLineList that might be relevant here. Not sure if something else obvious jumps out from that profile.
|
|
||
Comment 7•4 months ago
|
||
I think what you're seeing in that page is most likely bug 1901515.
|
|
Reporter | |
Comment 9•4 months ago
•
|
||
Always from that profile, if I interpret well during ~PresShell we really just free memory without calling any other dtors, and it is taking a significant amount of time. I think we could defer this with ClearOnShutdown ?
|
|
Reporter | |
Comment 10•4 months ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
I think what you're seeing in that page is most likely bug 1901515.
Not sure if bug 1901515 is supposed to change that, too, but in that profile I also see many direct destructions (free) of nsIFrame derived things which I would have expected to happen later looking here as we are probably seeing a mIsDestroying case? But maybe I miss quite a lot here, those might be other things held by those objects, too.
Edit: Ah, running destructors does not necessarily mean we free the memory and it seems those destructor runs cost us some time. Still I see the memory decreasing quite a lot while nsFrameManager::Destroy runs before we get to the actual nsDocumentViewer::DestroyPresShell.
|
|
Reporter | |
Comment 11•4 months ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
I think what you're seeing in that page is most likely bug 1901515.
I see it got a bit better with searchfox/IndexedDB but the time needed to process it is still notable. Besides the block I am proposing to defer in bug 1902728, the other most notable pieces are:
mozilla::dom::Document::ClearStaleServoData- sounds a bit like just memory clearing? Can we defer that, too?nsFrameList::DestroyFrames- unclear to me if that touches also things outside the frame tree? Probably yes?nsIContent::UnbindFromTree- running afterDestroyFramessuggests it does not need the frames? Would it be enough to just swap in an empty frame list?
From the names I can imagine we want nsIContent::UnbindFromTree to run sync, for the other two I do not know but it sounds a bit as if we could defer some of it.
|
|
||
Comment 12•4 months ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #11)
mozilla::dom::Document::ClearStaleServoData- sounds a bit like just memory clearing? Can we defer that, too?
The servo data clearing may be removable nowadays, it was added in bug 1414999 but we no longer arena-allocate style structs on the pres shell so possibly fine. We might need to relax this assert.
nsFrameList::DestroyFrames- unclear to me if that touches also things outside the frame tree? Probably yes?
Not sure what your question is exactly? We rely on frames to go away before unbind tho.
nsIContent::UnbindFromTree- running afterDestroyFramessuggests it does not need the frames? Would it be enough to just swap in an empty frame list?
We need to destroy the frames before unbinding because we need the space of the frame pointer to store the subtree root pointer for unbound nodes.
|
|
Reporter | |
Comment 13•4 months ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)
(In reply to Jens Stutte [:jstutte] from comment #11)
mozilla::dom::Document::ClearStaleServoData- sounds a bit like just memory clearing? Can we defer that, too?The servo data clearing may be removable nowadays, it was added in bug 1414999 but we no longer arena-allocate style structs on the pres shell so possibly fine. We might need to relax this assert.
OK, I assume "removable" means deferable, not omitted entirely.
nsFrameList::DestroyFrames- unclear to me if that touches also things outside the frame tree? Probably yes?Not sure what your question is exactly? We rely on frames to go away before unbind tho.
The question is if whatever the frames do on destruction has consequences only inside the same tree of things that get destroyed or if other things that will survive this DestroyFrames will get updated in between. So if "frames to go away before unbind" could just be "swap in an empty set and keep the old pointers alive elsewhere until deferred destruction".
nsIContent::UnbindFromTree- running afterDestroyFramessuggests it does not need the frames? Would it be enough to just swap in an empty frame list?We need to destroy the frames before unbinding because we need the space of the frame pointer to store the subtree root pointer for unbound nodes.
I read this as "DestroyFrames updates structures that then are needed to correctly do nsIContent::UnbindFromTree", so we cannot defer the whole thing but would need to seperate the "update other things" part of DestroyFrames from the "destroy myself" part, which sounds like too much work to be just done.
Description
•