Oilpan: enable eager tracing.

Oilpan: enable eager tracing.

Switch the GC marking phase to by default eagerly invoke the trace()
callbacks of unmarked objects.

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186820

[sof, 2014-12-09 12:10:23]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-12-09 12:10:23]

Please take a look.

Cannot go ahead until https://codereview.chromium.org/758073009/ lands - will
re-run oilpan trybots then - but otherwise considered ready.

[haraken, 2014-12-09 15:47:23]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-12-09 15:47:24]

Don't we need to land the stack depth check first? Remember that Oilpan is
already enabled by default for modules objects on trunk (which can potentially
cause stack overflow).

Also can we add a HeapTest that causes a stack overflow (and fall  back to a
manual marking stack)?

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:227: #define WILL_BE_EAGERLY_TRACED(TYPE)        
                                               \

This macro is no longer needed probably?

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:234: #define WILL_NOT_BE_EAGERLY_TRACED(TYPE)    
                                               \

Ditto.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:242: #define WILL_BE_EAGERLY_TRACED_CLASS(TYPE)  
    \

Ditto. If it's unused, we might want to drop this.

[Erik Corry, 2014-12-09 15:49:01]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-12-09 15:49:02]

LGTM

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:989: static const bool value =
TraceEagerlyTrait<T>::value || TraceEagerlyTrait<U>::value;
Can't this just be "true"?  We can eagerly trace the collection itself even if
its backing contains pointers that cannot be eagerly traced.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:513: inline void incrementTraceDepth() {
m_traceDepth++; }
Perhaps not in this CL, but it would be great to test the stack pointer hack
from
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/RiWQ8Gy_dBA/PRv...
to avoid the increment and decrement.

[sof, 2014-12-09 15:54:23]

On 2014/12/09 15:47:24, haraken wrote:
> Don't we need to land the stack depth check first? Remember that Oilpan is
> already enabled by default for modules objects on trunk (which can potentially
> cause stack overflow).
> 

It is already in place?


https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Question is what to do about the assert; silently accept the switch to explicit
marking or not.

[sof, 2014-12-09 15:56:40]

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:513: inline void incrementTraceDepth() {
m_traceDepth++; }
On 2014/12/09 15:49:02, Erik Corry wrote:
> Perhaps not in this CL, but it would be great to test the stack pointer hack
> from
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/RiWQ8Gy_dBA/PRv...
> to avoid the increment and decrement.

Numbers from 

 
https://docs.google.com/spreadsheets/d/1ff-QKXNne6TaWbbAqmmoDQdAeEay2nDDgpYNE...

suggests there is not much of a difference between the two.

[haraken, 2014-12-09 16:10:57]

On 2014/12/09 15:54:23, sof wrote:
> On 2014/12/09 15:47:24, haraken wrote:
> > Don't we need to land the stack depth check first? Remember that Oilpan is
> > already enabled by default for modules objects on trunk (which can
potentially
> > cause stack overflow).
> > 
> 
> It is already in place?
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Oh, thanks!

> Question is what to do about the assert; silently accept the switch to
explicit
> marking or not.

I'm fine with the current approach -- having an assert in ENABLE(ASSERT) and
silently falling back to a manual stack in !ENABLE(ASSERT).

Can we have a HeapTest for the silent fallback? We can protect the test with
!ENABLE(ASSERT).

LGTM.

[sof, 2014-12-09 21:55:37]

Added a HeapTest for explicit marking stack fallback as well.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:989: static const bool value =
TraceEagerlyTrait<T>::value || TraceEagerlyTrait<U>::value;
On 2014/12/09 15:49:02, Erik Corry wrote:
> Can't this just be "true"?  We can eagerly trace the collection itself even if
> its backing contains pointers that cannot be eagerly traced.

why not. I've made the enabled flag/setting for this feature take precedence, so
as to enable eager tracing with its current setting.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:227: #define WILL_BE_EAGERLY_TRACED(TYPE)        
                                               \
On 2014/12/09 15:47:24, haraken wrote:
> 
> This macro is no longer needed probably?
> 

No particular harm in having it around, but now unused on trunk. Hence removed.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:234: #define WILL_NOT_BE_EAGERLY_TRACED(TYPE)    
                                               \
On 2014/12/09 15:47:24, haraken wrote:
> 
> Ditto.

Kept, as it serves a purpose.

https://codereview.chromium.org/783823002/diff/20001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:242: #define WILL_BE_EAGERLY_TRACED_CLASS(TYPE)  
    \
On 2014/12/09 15:47:24, haraken wrote:
> 
> Ditto. If it's unused, we might want to drop this.

Retired also.

[sof, 2014-12-09 23:23:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-12-09 23:24:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/783823002/40001

[commit-bot: I haz the power, 2014-12-10 01:04:10]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186820