Issue 398793002: Oilpan: Remove support for tracing of off-heap ListHashSets.

Issue 398793002: Oilpan: Remove support for tracing of off-heap ListHashSets. (Closed)

Created:
6 years, 5 months ago by Mads Ager (chromium)

Modified:
6 years, 5 months ago

Reviewers:
haraken, oilpan-reviews, wibling-chromium

CC:
blink-reviews, shans, eae+blinkwatch, apavlov+blink_chromium.org, aandrey+blink_chromium.org, rune+blink, Yoav Weiss, Steve Block, blink-reviews-dom_chromium.org, blink-reviews-css, blink-reviews-html_chromium.org, dglazkov+blink, dstockwell, Timothy Loh, Rik, rwlbuis, Eric Willigers, kenneth.christiansen, rjwright, sof, darktears, blink-reviews-animation_chromium.org, Mike Lawther (Google), ed+blinkwatch_opera.com, kouhei+heap_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Oilpan: Remove support for tracing of off-heap ListHashSets. R=haraken@chromium.org, oilpan-reviews@chromium.org, wibling@chromium.org BUG=382557 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178353

Patch Set 1 #

Patch Set 2 : Fix number in test. #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+18 lines, -30 lines)			Patch
M	Source/core/css/CSSSegmentedFontFace.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/css/FontFaceCache.cpp	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/css/MediaQueryList.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/css/resolver/StyleResolver.cpp	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/dom/DocumentOrderedList.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/dom/ScriptedAnimationController.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/events/DOMWindowEventQueue.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/html/canvas/HitRegion.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/html/forms/FormController.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/platform/heap/HeapTest.cpp	View	1	8 chunks	+1 line, -9 lines	0 comments	Download
M	Source/platform/heap/Visitor.h	View	1	2 chunks	+0 lines, -20 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

haraken

- Are we planning to force the following rule? void trace(Visitor* visitor) { #if ENABLE(OILPAN) ...

6 years, 5 months ago (2014-07-16 13:14:28 UTC) #2

haraken

On 2014/07/16 13:14:28, haraken wrote: > - Are we planning to force the following rule? ...

6 years, 5 months ago (2014-07-16 13:16:07 UTC) #3

Mads Ager (chromium)

On 2014/07/16 13:16:07, haraken wrote: > On 2014/07/16 13:14:28, haraken wrote: > > - Are ...

6 years, 5 months ago (2014-07-16 13:23:11 UTC) #4

haraken

On 2014/07/16 13:23:11, Mads Ager (chromium) wrote: > On 2014/07/16 13:16:07, haraken wrote: > > ...

6 years, 5 months ago (2014-07-16 13:30:48 UTC) #5

On 2014/07/16 13:23:11, Mads Ager (chromium) wrote:
> On 2014/07/16 13:16:07, haraken wrote:
> > On 2014/07/16 13:14:28, haraken wrote:
> > > - Are we planning to force the following rule?
> > > 
> > > void trace(Visitor* visitor) {
> > > #if ENABLE(OILPAN)
> > >   visitor->trace(m_willBeObject1);
> > >   visitor->trace(m_willBeObject2);
> > >   visitor->trace(m_willBeObject3);
> > > #endif
> > >   visitor->trace(m_onHeapObject1);
> > >   visitor->trace(m_onHeapObject2);
> > >   visitor->trace(m_onHeapObject3);
> > > }
> > > 
> > > - Isn't there any way to only forbid tracing off-heap collections of
on-heap
> > > objects (so that we can avoid adding #if ENABLE(OILPAN) to a ton of
places)?
> > > 
> > > Vector<OnHeapObject>  => not allowed
> > > Vector<OffHeapObject>  => allowed, tracing is not needed but should be
> allowed
> > > HeapVector<OnHeapObject>  => allowed, tracing is needed
> > > HeapVector<OffHeapObject>  => not allowed
> > > WillBeHeapVector<OnHeapObject> (with oilpan enabled)  => allowed, tracing
is
> > > needed
> > > WillBeHeapVector<OffHeapObject> (with oilpan enabled)  => not allowed
> > > WillBeHeapVector<OnHeapObject> (with oilpan disabled)  => allowed, tracing
> is
> > > not needed but should be allowed
> > > WillBeHeapVector<OffHeapObject> (with oilpan disabled)  => not allowed
> > 
> > ^^^ Probably I wrote something confusing. The point of my question is
whether
> > there is a way to avoid adding #if ENABLE(OILPAN) to
> > visitor->trace(m_willBeCollections)?
> 
> We could add noop trace methods to the visitor for them. I'm not sure it is
> worth spending effort avoiding #if ENABLE(OILPAN) in our trace methods. In
your
> example above the #ifdef actually makes it clear that this method has
different
> meanings depending on the build.

My concern is:

- We need to add ENABLE(OILPAN) to visitor->trace(m_willBeVector), but don't
need to add ENABLE(OILPAN) to visitor->trace(m_willBeMember). We will mix these
in the code base and it's a bit confusing.

- We have to remove ENABLE(OILPAN) from visitor->trace(m_willBeMember) when
changing the m_willBeMember to m_member. At this point, if we forget to remove
ENABLE(OILPAN) and if the plugin had a bug, we will end up missing tracing the
member.

So if it's not that hard to add noop trace methods to the visitor, I'd prefer
the idea. What do you think?

Mads Ager (chromium)

On 2014/07/16 13:30:48, haraken wrote: > On 2014/07/16 13:23:11, Mads Ager (chromium) wrote: > > ...

6 years, 5 months ago (2014-07-16 13:43:01 UTC) #6

On 2014/07/16 13:30:48, haraken wrote:
> On 2014/07/16 13:23:11, Mads Ager (chromium) wrote:
> > On 2014/07/16 13:16:07, haraken wrote:
> > > On 2014/07/16 13:14:28, haraken wrote:
> > > > - Are we planning to force the following rule?
> > > > 
> > > > void trace(Visitor* visitor) {
> > > > #if ENABLE(OILPAN)
> > > >   visitor->trace(m_willBeObject1);
> > > >   visitor->trace(m_willBeObject2);
> > > >   visitor->trace(m_willBeObject3);
> > > > #endif
> > > >   visitor->trace(m_onHeapObject1);
> > > >   visitor->trace(m_onHeapObject2);
> > > >   visitor->trace(m_onHeapObject3);
> > > > }
> > > > 
> > > > - Isn't there any way to only forbid tracing off-heap collections of
> on-heap
> > > > objects (so that we can avoid adding #if ENABLE(OILPAN) to a ton of
> places)?
> > > > 
> > > > Vector<OnHeapObject>  => not allowed
> > > > Vector<OffHeapObject>  => allowed, tracing is not needed but should be
> > allowed
> > > > HeapVector<OnHeapObject>  => allowed, tracing is needed
> > > > HeapVector<OffHeapObject>  => not allowed
> > > > WillBeHeapVector<OnHeapObject> (with oilpan enabled)  => allowed,
tracing
> is
> > > > needed
> > > > WillBeHeapVector<OffHeapObject> (with oilpan enabled)  => not allowed
> > > > WillBeHeapVector<OnHeapObject> (with oilpan disabled)  => allowed,
tracing
> > is
> > > > not needed but should be allowed
> > > > WillBeHeapVector<OffHeapObject> (with oilpan disabled)  => not allowed
> > > 
> > > ^^^ Probably I wrote something confusing. The point of my question is
> whether
> > > there is a way to avoid adding #if ENABLE(OILPAN) to
> > > visitor->trace(m_willBeCollections)?
> > 
> > We could add noop trace methods to the visitor for them. I'm not sure it is
> > worth spending effort avoiding #if ENABLE(OILPAN) in our trace methods. In
> your
> > example above the #ifdef actually makes it clear that this method has
> different
> > meanings depending on the build.
> 
> My concern is:
> 
> - We need to add ENABLE(OILPAN) to visitor->trace(m_willBeVector), but don't
> need to add ENABLE(OILPAN) to visitor->trace(m_willBeMember). We will mix
these
> in the code base and it's a bit confusing.
> 
> - We have to remove ENABLE(OILPAN) from visitor->trace(m_willBeMember) when
> changing the m_willBeMember to m_member. At this point, if we forget to remove
> ENABLE(OILPAN) and if the plugin had a bug, we will end up missing tracing the
> member.
> 
> So if it's not that hard to add noop trace methods to the visitor, I'd prefer
> the idea. What do you think?

I think I have the opposite preference. :)

I'm worried about having these noop trace methods that allow you to call trace
on an off-heap collection. If you have an object that is in the oilpan heap by
default, you could by accident add an off-heap collection of pointers to this
object, call trace on it and the trace method would do nothing. That could lead
to dangling pointers. We could probably extend the plugin to catch this, but
then we are relying on the plugin in exactly the same way we are now. What I
have done here works with the current plugin verification which I think is good.

I'm afraid there is no really satisfying way out here.

haraken

On 2014/07/16 13:43:01, Mads Ager (chromium) wrote: > On 2014/07/16 13:30:48, haraken wrote: > > ...

6 years, 5 months ago (2014-07-16 14:00:53 UTC) #7

On 2014/07/16 13:43:01, Mads Ager (chromium) wrote:
> On 2014/07/16 13:30:48, haraken wrote:
> > On 2014/07/16 13:23:11, Mads Ager (chromium) wrote:
> > > On 2014/07/16 13:16:07, haraken wrote:
> > > > On 2014/07/16 13:14:28, haraken wrote:
> > > > > - Are we planning to force the following rule?
> > > > > 
> > > > > void trace(Visitor* visitor) {
> > > > > #if ENABLE(OILPAN)
> > > > >   visitor->trace(m_willBeObject1);
> > > > >   visitor->trace(m_willBeObject2);
> > > > >   visitor->trace(m_willBeObject3);
> > > > > #endif
> > > > >   visitor->trace(m_onHeapObject1);
> > > > >   visitor->trace(m_onHeapObject2);
> > > > >   visitor->trace(m_onHeapObject3);
> > > > > }
> > > > > 
> > > > > - Isn't there any way to only forbid tracing off-heap collections of
> > on-heap
> > > > > objects (so that we can avoid adding #if ENABLE(OILPAN) to a ton of
> > places)?
> > > > > 
> > > > > Vector<OnHeapObject>  => not allowed
> > > > > Vector<OffHeapObject>  => allowed, tracing is not needed but should be
> > > allowed
> > > > > HeapVector<OnHeapObject>  => allowed, tracing is needed
> > > > > HeapVector<OffHeapObject>  => not allowed
> > > > > WillBeHeapVector<OnHeapObject> (with oilpan enabled)  => allowed,
> tracing
> > is
> > > > > needed
> > > > > WillBeHeapVector<OffHeapObject> (with oilpan enabled)  => not allowed
> > > > > WillBeHeapVector<OnHeapObject> (with oilpan disabled)  => allowed,
> tracing
> > > is
> > > > > not needed but should be allowed
> > > > > WillBeHeapVector<OffHeapObject> (with oilpan disabled)  => not allowed
> > > > 
> > > > ^^^ Probably I wrote something confusing. The point of my question is
> > whether
> > > > there is a way to avoid adding #if ENABLE(OILPAN) to
> > > > visitor->trace(m_willBeCollections)?
> > > 
> > > We could add noop trace methods to the visitor for them. I'm not sure it
is
> > > worth spending effort avoiding #if ENABLE(OILPAN) in our trace methods. In
> > your
> > > example above the #ifdef actually makes it clear that this method has
> > different
> > > meanings depending on the build.
> > 
> > My concern is:
> > 
> > - We need to add ENABLE(OILPAN) to visitor->trace(m_willBeVector), but don't
> > need to add ENABLE(OILPAN) to visitor->trace(m_willBeMember). We will mix
> these
> > in the code base and it's a bit confusing.
> > 
> > - We have to remove ENABLE(OILPAN) from visitor->trace(m_willBeMember) when
> > changing the m_willBeMember to m_member. At this point, if we forget to
remove
> > ENABLE(OILPAN) and if the plugin had a bug, we will end up missing tracing
the
> > member.
> > 
> > So if it's not that hard to add noop trace methods to the visitor, I'd
prefer
> > the idea. What do you think?
> 
> I think I have the opposite preference. :)
> 
> I'm worried about having these noop trace methods that allow you to call trace
> on an off-heap collection. If you have an object that is in the oilpan heap by
> default, you could by accident add an off-heap collection of pointers to this
> object, call trace on it and the trace method would do nothing. That could
lead
> to dangling pointers. We could probably extend the plugin to catch this, but
> then we are relying on the plugin in exactly the same way we are now. What I
> have done here works with the current plugin verification which I think is
good.
> 
> I'm afraid there is no really satisfying way out here.

OK, makes sense. LGTM.

Probably we want to limit ENABLE(OILPAN) for
visitor->trace(m_willBeCollections). (i.e., I mean we want to write
visitor->trace(m_willBeMember) outside ENABLE(OILPAN).)

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/398793002/20001

6 years, 5 months ago (2014-07-17 08:57:38 UTC) #9

Message was sent while issue was closed.

Change committed as 178353

Expand Messages | Collapse Messages