Oilpan: handle thread-local weak tracing slightly better.

Oilpan: handle thread-local weak tracing slightly better.

The implementation of marking is marking-thread centric, recording a
thread stack limit that the tracing through the object graph cannot
exceed. If it does, the implementation will use the explicit mark
stack instead of trying to eagerly trace the object, if unmarked.

Tracing will in the general case also happen on a per-thread basis,
when handling the weak pointer callbacks after the initial marking
phase has completed. Using the marking thread's stack limit to determine
eagerness or not of a trace call when that weak tracing runs doesn't
make much sense.

To address, eager tracing is not enabled whilst processing the
per-thread weak pointer callbacks. As the amount of tracing is rather
moderate for this stage, there are no performance concerns with
disabling this optimization.

R=haraken
BUG=420515
TEST=HeapTest.ThreadedStrongification

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

[sof, 2015-05-04 07:08:14]

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

[sof, 2015-05-04 07:08:14]

please take a look.

The weak marking step doesn't appear accounted for by the stack-based eager
tracing depth checking now in place. I can be convinced that we need a more
reliable change than this (== no eager tracing for thread local weak tracing),
but this will be adequate for the time being. wdyt?

It addresses stuff like


https://storage.googleapis.com/chromium-layout-test-archives/win_blink_oilpan...

[sof, 2015-05-04 09:53:28]

On 2015/05/04 07:08:14, sof wrote:
> please take a look.
> 
> The weak marking step doesn't appear accounted for by the stack-based eager
> tracing depth checking now in place. I can be convinced that we need a more
> reliable change than this (== no eager tracing for thread local weak tracing),
> but this will be adequate for the time being. wdyt?
> 

Upon further thought, it would preferable not to be too loose about these stack
checks. ps#3 disables eager tracing for thread-local weak processing.

[haraken, 2015-05-04 23:11:31]

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

[haraken, 2015-05-04 23:11:32]

Thanks for looking into this.

> Upon further thought, it would preferable not to be too loose about these
stack
> checks. ps#3 disables eager tracing for thread-local weak processing.

I haven't looked into the details but my impression is that it is wrong that a
mark method is called during thread-local weak processing. Why do we need to
mark an object during a thread-local weak processing?

As far as I scan the crash trace, the mark method is called in a weak processing
for hash tables. Maybe the implementation of TraceInCollection is not right?

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.h (right):

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.h:34: static void clearStackLimit()

configureStackLimit => enableStackLimit
clearStackLimit => disableStackLimit
m_isActive => m_enabled

?

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.h:78: class StackFrameDepthScope {

Add STACK_ALLOCATED.

[sof, 2015-05-05 05:28:08]

On 2015/05/04 23:11:32, haraken wrote:
> Thanks for looking into this.
> 
> > Upon further thought, it would preferable not to be too loose about these
> stack
> > checks. ps#3 disables eager tracing for thread-local weak processing.
> 
> I haven't looked into the details but my impression is that it is wrong that a
> mark method is called during thread-local weak processing. Why do we need to
> mark an object during a thread-local weak processing?
> 

I don't understand - the value of a weak key will only be kept alive if the weak
key is, so why wouldn't you mark & trace at this stage?

[sof, 2015-05-05 06:27:04]

On 2015/05/05 05:28:08, sof wrote:
> On 2015/05/04 23:11:32, haraken wrote:
> > Thanks for looking into this.
> > 
> > > Upon further thought, it would preferable not to be too loose about these
> > stack
> > > checks. ps#3 disables eager tracing for thread-local weak processing.
> > 
> > I haven't looked into the details but my impression is that it is wrong that
a
> > mark method is called during thread-local weak processing. Why do we need to
> > mark an object during a thread-local weak processing?
> > 
> 
> I don't understand - the value of a weak key will only be kept alive if the
weak
> key is, so why wouldn't you mark & trace at this stage?

Allow me to 'weaken' that requirement a bit? :) The handling of weaks in
collections will have done almost all of the work by the time thread-local weak
processing happens, but the implementation still has to check liveness status of
(e.g.) the keys & values of a hash map so as to remove entries from the hash
maps. And that needs to be done by way of tracing in the general case.

[haraken, 2015-05-05 10:44:05]

On 2015/05/05 06:27:04, sof wrote:
> On 2015/05/05 05:28:08, sof wrote:
> > On 2015/05/04 23:11:32, haraken wrote:
> > > Thanks for looking into this.
> > > 
> > > > Upon further thought, it would preferable not to be too loose about
these
> > > stack
> > > > checks. ps#3 disables eager tracing for thread-local weak processing.
> > > 
> > > I haven't looked into the details but my impression is that it is wrong
that
> a
> > > mark method is called during thread-local weak processing. Why do we need
to
> > > mark an object during a thread-local weak processing?
> > > 
> > 
> > I don't understand - the value of a weak key will only be kept alive if the
> weak
> > key is, so why wouldn't you mark & trace at this stage?
> 
> Allow me to 'weaken' that requirement a bit? :) The handling of weaks in
> collections will have done almost all of the work by the time thread-local
weak
> processing happens, but the implementation still has to check liveness status
of
> (e.g.) the keys & values of a hash map so as to remove entries from the hash
> maps. And that needs to be done by way of tracing in the general case.

Just to confirm: The tracing that happens during thread-local weak processing
just traverses objects that already have mark bits, and thus the tracing
shouldn't mark any new object. Is my understanding correct?

Either way this CL LGTM.

[sof, 2015-05-05 11:13:19]

On 2015/05/05 10:44:05, haraken wrote:
> On 2015/05/05 06:27:04, sof wrote:
> > On 2015/05/05 05:28:08, sof wrote:
> > > On 2015/05/04 23:11:32, haraken wrote:
> > > > Thanks for looking into this.
> > > > 
> > > > > Upon further thought, it would preferable not to be too loose about
> these
> > > > stack
> > > > > checks. ps#3 disables eager tracing for thread-local weak processing.
> > > > 
> > > > I haven't looked into the details but my impression is that it is wrong
> that
> > a
> > > > mark method is called during thread-local weak processing. Why do we
need
> to
> > > > mark an object during a thread-local weak processing?
> > > > 
> > > 
> > > I don't understand - the value of a weak key will only be kept alive if
the
> > weak
> > > key is, so why wouldn't you mark & trace at this stage?
> > 
> > Allow me to 'weaken' that requirement a bit? :) The handling of weaks in
> > collections will have done almost all of the work by the time thread-local
> weak
> > processing happens, but the implementation still has to check liveness
status
> of
> > (e.g.) the keys & values of a hash map so as to remove entries from the hash
> > maps. And that needs to be done by way of tracing in the general case.
> 
> Just to confirm: The tracing that happens during thread-local weak processing
> just traverses objects that already have mark bits, and thus the tracing
> shouldn't mark any new object. Is my understanding correct?
> 

That matches my (so far incomplete) understanding. I think we cannot instead
rely on isHeapObjectAlive() in the general (nested) case, but haven't gone after
the details.

[haraken, 2015-05-05 16:04:18]

On 2015/05/05 11:13:19, sof wrote:
> On 2015/05/05 10:44:05, haraken wrote:
> > On 2015/05/05 06:27:04, sof wrote:
> > > On 2015/05/05 05:28:08, sof wrote:
> > > > On 2015/05/04 23:11:32, haraken wrote:
> > > > > Thanks for looking into this.
> > > > > 
> > > > > > Upon further thought, it would preferable not to be too loose about
> > these
> > > > > stack
> > > > > > checks. ps#3 disables eager tracing for thread-local weak
processing.
> > > > > 
> > > > > I haven't looked into the details but my impression is that it is
wrong
> > that
> > > a
> > > > > mark method is called during thread-local weak processing. Why do we
> need
> > to
> > > > > mark an object during a thread-local weak processing?
> > > > > 
> > > > 
> > > > I don't understand - the value of a weak key will only be kept alive if
> the
> > > weak
> > > > key is, so why wouldn't you mark & trace at this stage?
> > > 
> > > Allow me to 'weaken' that requirement a bit? :) The handling of weaks in
> > > collections will have done almost all of the work by the time thread-local
> > weak
> > > processing happens, but the implementation still has to check liveness
> status
> > of
> > > (e.g.) the keys & values of a hash map so as to remove entries from the
hash
> > > maps. And that needs to be done by way of tracing in the general case.
> > 
> > Just to confirm: The tracing that happens during thread-local weak
processing
> > just traverses objects that already have mark bits, and thus the tracing
> > shouldn't mark any new object. Is my understanding correct?
> > 
> 
> That matches my (so far incomplete) understanding. I think we cannot instead
> rely on isHeapObjectAlive() in the general (nested) case, but haven't gone
after
> the details.

OK, I also need to understand how the weak processing for HashTable is working.
Either way I think this CL is doing a right thing.

[sof, 2015-05-06 10:39:57]

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.h (right):

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.h:34: static void clearStackLimit()
On 2015/05/04 23:11:32, haraken wrote:
> 
> configureStackLimit => enableStackLimit
> clearStackLimit => disableStackLimit
> m_isActive => m_enabled
> 
> ?

Renamed so.

https://codereview.chromium.org/1125613002/diff/60001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.h:78: class StackFrameDepthScope {
On 2015/05/04 23:11:32, haraken wrote:
> 
> Add STACK_ALLOCATED.

Not done, that would be an unnecessary restriction.

[sof, 2015-05-06 10:40:04]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-06 10:40:05]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1125613002/#ps80001
(title: "rename some methods")

[commit-bot: I haz the power, 2015-05-06 10:40:10]

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

[commit-bot: I haz the power, 2015-05-06 12:30:50]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194970