StackFrameDepth should be managed per ThreadHeap

StackFrameDepth should be managed per ThreadHeap

StackFrameDepth should be managed per ThreadHeap so we won't have issues when we run multiple GCs simultaniously.

BUG=

Committed: https://crrev.com/098b330a204a365b70d381ca9b3e51a9ca7efc29
Cr-Commit-Position: refs/heads/master@{#416908}

[keishi, 2016-09-02 06:42:02]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 06:42:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[keishi, 2016-09-02 06:42:30]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-09-02 06:42:31]

PTAL

[commit-bot: I haz the power, 2016-09-02 06:59:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 06:59:31]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[haraken, 2016-09-02 09:07:58]

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/StackFrameDepth.h (right):

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/StackFrameDepth.h:22: class
PLATFORM_EXPORT StackFrameDepth final {

Add DISALLOW_NEW().

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/StackFrameDepth.h:94: class
StackFrameDepthScope {

Conceptually StackFrameDepthScope should be equal to the scope of Visitor. It
may be better to move the contents of StackFrameDepthScope to Visitor and remove
StackFrameDepthScope.

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/StackFrameDepth.h:98:
StackFrameDepthScope(StackFrameDepth* depth): m_depth(depth)

Add explicit.

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
(LIKELY(visitor->heap().stackFrameDepth().isSafeToRecurse())) {

I hope this won't regress performance.

Moving StackFrameDepthScope to Visitor might help the compiler to generate more
optimized code.

[haraken, 2016-09-02 09:08:49]

On 2016/09/02 09:07:58, haraken wrote:
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/heap/StackFrameDepth.h (right):
> 
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/StackFrameDepth.h:22: class
> PLATFORM_EXPORT StackFrameDepth final {
> 
> Add DISALLOW_NEW().
> 
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/StackFrameDepth.h:94: class
> StackFrameDepthScope {
> 
> Conceptually StackFrameDepthScope should be equal to the scope of Visitor. It
> may be better to move the contents of StackFrameDepthScope to Visitor and
remove
> StackFrameDepthScope.
> 
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/StackFrameDepth.h:98:
> StackFrameDepthScope(StackFrameDepth* depth): m_depth(depth)
> 
> Add explicit.
> 
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):
> 
>
https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
> (LIKELY(visitor->heap().stackFrameDepth().isSafeToRecurse())) {
> 
> I hope this won't regress performance.
> 
> Moving StackFrameDepthScope to Visitor might help the compiler to generate
more
> optimized code.

BTW, how did you notice that you need to move StackFrameDepth to per-thread?

[keishi, 2016-09-02 09:32:02]

I kept hitting the ASSERT in StackFrameDepth::isSafeToRecurse() sporadically
when I triggered GC on multiple threads from collectAllGarbage in the other CL.

[keishi, 2016-09-05 14:00:30]

CL for src perf tryjob to run sunspider benchmark on all platform(s)

[keishi, 2016-09-05 14:02:00]

CL for src perf tryjob to run oilpan_gc_times.blink_perf_stress benchmark on all
platform(s)

[keishi, 2016-09-06 12:54:22]

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/StackFrameDepth.h (right):

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/StackFrameDepth.h:94: class
StackFrameDepthScope {
On 2016/09/02 09:07:57, haraken wrote:
> 
> Conceptually StackFrameDepthScope should be equal to the scope of Visitor. It
> may be better to move the contents of StackFrameDepthScope to Visitor and
remove
> StackFrameDepthScope.
> 

Done.

https://codereview.chromium.org/2304023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/StackFrameDepth.h:98:
StackFrameDepthScope(StackFrameDepth* depth): m_depth(depth)
On 2016/09/02 09:07:57, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
(LIKELY(visitor->isAcceptableStackUse())) {
I saw a 30% regression with oilpan_gc_times.blink_perf_stress if I use
isSafeToRecurse.
I think the difference in initialization between s_stackFrameLimit and
m_stackFrameLimit made the LIKELY not true. If I use isAcceptableStackUse I
don't get any regression.

[haraken, 2016-09-06 13:03:11]

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
(LIKELY(visitor->isAcceptableStackUse())) {
On 2016/09/06 12:54:22, keishi wrote:
> I saw a 30% regression with oilpan_gc_times.blink_perf_stress if I use
> isSafeToRecurse.
> I think the difference in initialization between s_stackFrameLimit and
> m_stackFrameLimit made the LIKELY not true. If I use isAcceptableStackUse I
> don't get any regression.

Hmm? isAcceptableStackUse is just calling isSafeToRecurse and isEnabled. Also
isEnabled() is true during marking.

Why does replacing isSafeToRecurse with isAcceptableStackUse change the
likelihood where you hit the branch?

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Visitor.h:121: class VisitorHelper :
public StackFrameDepth {

This looks like a weird class design.

This is okay in this CL but consider making Visitor(Helper) own the
StackFrameDepth.

[keishi, 2016-09-07 05:38:21]

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
(LIKELY(visitor->isAcceptableStackUse())) {
On 2016/09/06 13:03:11, haraken wrote:
> On 2016/09/06 12:54:22, keishi wrote:
> > I saw a 30% regression with oilpan_gc_times.blink_perf_stress if I use
> > isSafeToRecurse.
> > I think the difference in initialization between s_stackFrameLimit and
> > m_stackFrameLimit made the LIKELY not true. If I use isAcceptableStackUse I
> > don't get any regression.
> 
> Hmm? isAcceptableStackUse is just calling isSafeToRecurse and isEnabled. Also
> isEnabled() is true during marking.
> 
> Why does replacing isSafeToRecurse with isAcceptableStackUse change the
> likelihood where you hit the branch?

I guess the problem with this PS is that it isn't preserving the StackFrameDepth
information when we create InlinedGlobalMarkingVisitor to switch to inlined
global marking dispatch.

If we have multiple Visitors per gc I don't think each visitor should have a
StackFrameDepth.

I think its best for ThreadHeap to own one StackFrameDepth like PS1.

[keishi, 2016-09-07 05:39:26]

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):

https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
(LIKELY(visitor->isAcceptableStackUse())) {
On 2016/09/06 13:03:11, haraken wrote:
> On 2016/09/06 12:54:22, keishi wrote:
> > I saw a 30% regression with oilpan_gc_times.blink_perf_stress if I use
> > isSafeToRecurse.
> > I think the difference in initialization between s_stackFrameLimit and
> > m_stackFrameLimit made the LIKELY not true. If I use isAcceptableStackUse I
> > don't get any regression.
> 
> Hmm? isAcceptableStackUse is just calling isSafeToRecurse and isEnabled. Also
> isEnabled() is true during marking.
> 
> Why does replacing isSafeToRecurse with isAcceptableStackUse change the
> likelihood where you hit the branch?

I guess the problem with this CL is that it isn't preserving the StackFrameDepth
information when we create InlinedGlobalMarkingVisitor to switch to inlined
global marking dispatch.

If we have multiple Visitors per gc I don't think each visitor should have a
StackFrameDepth.

I think its best for ThreadHeap to own one StackFrameDepth like PS1.
I ran speedometer and oilpan_gc_times.blink_perf_stress and I don't see any
performance regression at all.

[haraken, 2016-09-07 06:31:50]

On 2016/09/07 05:39:26, keishi wrote:
>
https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):
> 
>
https://codereview.chromium.org/2304023003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/heap/TraceTraits.h:74: if
> (LIKELY(visitor->isAcceptableStackUse())) {
> On 2016/09/06 13:03:11, haraken wrote:
> > On 2016/09/06 12:54:22, keishi wrote:
> > > I saw a 30% regression with oilpan_gc_times.blink_perf_stress if I use
> > > isSafeToRecurse.
> > > I think the difference in initialization between s_stackFrameLimit and
> > > m_stackFrameLimit made the LIKELY not true. If I use isAcceptableStackUse
I
> > > don't get any regression.
> > 
> > Hmm? isAcceptableStackUse is just calling isSafeToRecurse and isEnabled.
Also
> > isEnabled() is true during marking.
> > 
> > Why does replacing isSafeToRecurse with isAcceptableStackUse change the
> > likelihood where you hit the branch?
> 
> I guess the problem with this CL is that it isn't preserving the
StackFrameDepth
> information when we create InlinedGlobalMarkingVisitor to switch to inlined
> global marking dispatch.
> 
> If we have multiple Visitors per gc I don't think each visitor should have a
> StackFrameDepth.
> 
> I think its best for ThreadHeap to own one StackFrameDepth like PS1.
> I ran speedometer and oilpan_gc_times.blink_perf_stress and I don't see any
> performance regression at all.

Thanks, makes sense. LGTM.

[keishi, 2016-09-07 06:54:02]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-07 06:54:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 11:15:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-07 11:17:06]

Description was changed from

==========
StackFrameDepth should be managed per ThreadHeap

StackFrameDepth should be managed per ThreadHeap so we won't have issues when we
run multiple GCs simultaniously.

BUG=
==========

to

==========
StackFrameDepth should be managed per ThreadHeap

StackFrameDepth should be managed per ThreadHeap so we won't have issues when we
run multiple GCs simultaniously.

BUG=

Committed: https://crrev.com/098b330a204a365b70d381ca9b3e51a9ca7efc29
Cr-Commit-Position: refs/heads/master@{#416908}
==========

[commit-bot: I haz the power, 2016-09-07 11:17:07]

Patchset 7 (id:??) landed as
https://crrev.com/098b330a204a365b70d381ca9b3e51a9ca7efc29
Cr-Commit-Position: refs/heads/master@{#416908}

[keishi, 2016-09-09 05:02:08]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2326523004/ by keishi@chromium.org.

The reason for reverting is: Caused
FetchDataConsumerTeeTest.CreateFromBlobWithInvalidSize and
FetchDataConsumerTeeTest.Create to fail
https://bugs.chromium.org/p/chromium/issues/detail?id=645253.