Oilpan: Move DOMEditor to the heap

Oilpan: Move DOMEditor to the heap

BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175947

[pfeldman, 2014-05-22 14:47:20]

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.h:47: class DOMEditor FINAL : public
NoBaseWillBeGarbageCollected<DOMEditor> {
What is the motivation of this change? DOMEditor is a simple non-refcounted
class with clear lifetime.

[haraken, 2014-05-22 20:19:31]

LGTM with comments but let's wait for an approval from Pavel.

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.cpp (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.cpp:289: Node* m_newNode;

I think this should be RawPtrWillBeMember. (I wonder why this pointer was not a
RefPtr before.)

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.h:47: class DOMEditor FINAL : public
NoBaseWillBeGarbageCollected<DOMEditor> {
On 2014/05/22 14:47:21, pfeldman wrote:
> What is the motivation of this change? DOMEditor is a simple non-refcounted
> class with clear lifetime.

- InspectorHistory is moved to the heap.
- So if we keep DOMEditor off-heap, we need to use a
Persistent<InspectorHistory> for DOMEditor::m_history.
- However, we don't want to increase the number of persistent handles, it would
be better to move DOMEditor to the heap at the same time and use
Member<DOMEditor> for DOMEditor::m_history.

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorHistory.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorHistory.h:50: Action(const String& name);

Add explicit.

[pfeldman, 2014-05-22 20:43:25]

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.h:47: class DOMEditor FINAL : public
NoBaseWillBeGarbageCollected<DOMEditor> {
Why moving history to heap? Because some actions point to soon-to-be heap
objects?

I am a bit concerned with conversion of simple non-hierarchical data structures
with clear lifetime such as DOMEditor into the Oilpan heap. It makes it hard to
follow the ownership and track the new object lifetimes.

So we need to draw the line somewhere and I am not sure I know what the
guidelines / criteria for that is. Or do we want to move as much as possible
into heap? Would LocalFrame stay a regular object?

[haraken, 2014-05-22 21:21:56]

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.h:47: class DOMEditor FINAL : public
NoBaseWillBeGarbageCollected<DOMEditor> {
> Why moving history to heap? Because some actions point to soon-to-be heap
> objects?

- We're currently moving the Node hierarchy to the heap.
- Because the Action hierarchy has a bunch of RefPtrs to the Node, we want to
move the Action hierarchy to the heap.
- Because InspectorHistory has a vector of Actions, we want to move
InspectorHistory to the heap.

If we don't move them to the heap at the same time, we need to use persistent
handles, which we want to avoid as much as possible.

> I am a bit concerned with conversion of simple non-hierarchical data
structures
> with clear lifetime such as DOMEditor into the Oilpan heap. It makes it hard
to
> follow the ownership and track the new object lifetimes.
> 
> So we need to draw the line somewhere and I am not sure I know what the
> guidelines / criteria for that is. Or do we want to move as much as possible
> into heap? Would LocalFrame stay a regular object?

In the end, we're planning to move:

- almost all RefCounted objects
- objects that have references to the RefCounted objects (Otherwise we need to
use persistent handles)
- strings and collections

to the heap.

The reason why we want to avoid persistent handles is that (1) persistent
handles are heavy in terms of both performance and memory and that (2)
persistent handles can cause leaks.

For example:

class A {
  Persistent<B> m_b;
};

class B : public GarbageCollectedFinalized<B> {
  OwnPtr<A> m_a;
};

will cause a cycle, so we need to move A to the heap at the same time:

class A : public GarbageCollected<A> {
  Member<B> m_b;
};

class B : public GarbageCollected<B> {
  Member<A> m_a;
};

[keishi, 2014-05-23 02:22:20]

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
File Source/core/inspector/DOMEditor.cpp (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/DO...
Source/core/inspector/DOMEditor.cpp:289: Node* m_newNode;
On 2014/05/22 20:19:31, haraken wrote:
> 
> I think this should be RawPtrWillBeMember. (I wonder why this pointer was not
a
> RefPtr before.)

Done.

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorHistory.h (right):

https://codereview.chromium.org/298873004/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorHistory.h:50: Action(const String& name);
On 2014/05/22 20:19:31, haraken wrote:
> 
> Add explicit.

Done.

[pfeldman, 2014-05-23 05:32:12]

> In the end, we're planning to move:
> 
> - almost all RefCounted objects
> - objects that have references to the RefCounted objects (Otherwise we need to
> use persistent handles)
> - strings and collections
> 
> to the heap.


Well, that's Blink up to a LocalFrame. My thinking is that LocalFrame / Page
will not be in Oilpan. InspectorController has matching lifetime and is
currently owned by Page. So it is also not in Oilpan heap. Same goes for
InspectorAgents since they live as long as InspectorController does. Now
DOMEditor and InspectorHistory are in fact extracted from one of these agents
simply to make the code more modular. They are created when debugging is engaged
and are deleted when it is over. So they also should not live in Oilpan heap.

So I guess persistent handles in actions that is. I don't see a leak as you
describe above possible due to the nature of the case. On contrary, I can see
large objects with clear lifetimes leaking on some accidental dangling oilpan
refs if they get there.

[Mads Ager (chromium), 2014-05-23 06:44:53]

On 2014/05/23 05:32:12, pfeldman wrote:
> > In the end, we're planning to move:
> > 
> > - almost all RefCounted objects
> > - objects that have references to the RefCounted objects (Otherwise we need
to
> > use persistent handles)
> > - strings and collections
> > 
> > to the heap.
> 
> 
> Well, that's Blink up to a LocalFrame. My thinking is that LocalFrame / Page
> will not be in Oilpan.

Page is already in the Oilpan heap. It has supplements and collections that need
weak processing.

> InspectorController has matching lifetime and is
> currently owned by Page. So it is also not in Oilpan heap. Same goes for
> InspectorAgents since they live as long as InspectorController does. Now
> DOMEditor and InspectorHistory are in fact extracted from one of these agents
> simply to make the code more modular. They are created when debugging is
engaged
> and are deleted when it is over. So they also should not live in Oilpan heap.

I understand where you are coming from Pavel. However, in order to use weak
processing, the owner object needs to be in the oilpan heap so that the oilpan
infrastructure knows about it and can perform weak processing for it. Therefore,
I would expect us to want to move most of the inspector objects to the oilpan
heap when we want to use real weak processing.

> So I guess persistent handles in actions that is. I don't see a leak as you
> describe above possible due to the nature of the case. On contrary, I can see
> large objects with clear lifetimes leaking on some accidental dangling oilpan
> refs if they get there.

The drawback of persistent handles is that they take up more space and that they
can lead to cycles. Using persistents is fine if making tasks a bit larger is
not a problem and we are convinced that we cannot get a cycles via a node that
keeps a task alive which uses a persistent to point back to a node in the same
hierarchy.

[pfeldman, 2014-05-23 08:14:24]

Page being a part of Oilpan heap is a convincing factor :) But it seems like
this change eventually introduces a memory leak that haraken@ refers to in
comment #5:

Page -> OwnPtr<InspectorController> -> OwnPtr<InspectorDOMAgent> ->
Persistent<DOMEditor> -> Member<InspectorHistory> -> Member<Some Node> ->
Document -> Page

So should we start with putting InspectorController and InspectorDOMAgent into
the Oilpan heap instead? Or are you doing it in a follow up?

[keishi, 2014-05-23 08:31:00]

On 2014/05/23 08:14:24, pfeldman wrote:
> Page being a part of Oilpan heap is a convincing factor :) But it seems like
> this change eventually introduces a memory leak that haraken@ refers to in
> comment #5:
> 
> Page -> OwnPtr<InspectorController> -> OwnPtr<InspectorDOMAgent> ->
> Persistent<DOMEditor> -> Member<InspectorHistory> -> Member<Some Node> ->
> Document -> Page
> 
> So should we start with putting InspectorController and InspectorDOMAgent into
> the Oilpan heap instead? Or are you doing it in a follow up?

I'm working on them right now.

[haraken, 2014-05-23 08:37:21]

On 2014/05/23 08:14:24, pfeldman wrote:
> Page being a part of Oilpan heap is a convincing factor :) But it seems like
> this change eventually introduces a memory leak that haraken@ refers to in
> comment #5:
> 
> Page -> OwnPtr<InspectorController> -> OwnPtr<InspectorDOMAgent> ->
> Persistent<DOMEditor> -> Member<InspectorHistory> -> Member<Some Node> ->
> Document -> Page
> 
> So should we start with putting InspectorController and InspectorDOMAgent into
> the Oilpan heap instead? Or are you doing it in a follow up?

Thanks Pavel. Yes, the plan is to remove all suspicious persistent handles by
moving more inspector objects to the heap. It might be possible that each change
will introduce an unexpected cycle and leak memory (only in oilpan builds), but
those leaks are supposed to be eliminated as we move more objects to the heap.
(Of course, we won't enable the oilpan flag until we eliminate all leaks.)

[pfeldman, 2014-05-23 08:47:24]

Alright, as long as you follow up on agents, lgtm. Thanks for explaining things
to me!

[haraken, 2014-06-11 05:36:00]

Shall we rebase this CL and push it to a commit queue?

[keishi, 2014-06-11 05:45:44]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-06-11 05:46:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/298873004/60001

[commit-bot: I haz the power, 2014-06-11 06:52:59]

Change committed as 175947