Allow creating non-thread-local Persistents that can be passed between threads.

Allow creating non-thread-local Persistents that can be passed between threads.

Before this change all persistents must have been created and destroyed on the same thread.

This change introduces Persistent<T, GlobalPersistents> type. Such persistent can be safely allocated and destroyed on any thread.

BUG=

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

[Vyacheslav Egorov (Chromium), 2014-02-17 14:35:04]

PTAL

[Mads Ager (chromium), 2014-02-17 15:00:12]

LGTM

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode93
Source/heap/Handle.h:93: // No locking requires. Just checks that we are at the
right thread.
required

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode132
Source/heap/Handle.h:132: typename RootsAccessor::Lock lock;
Shouldn't you lock before the debug-mode code? The debug mode code is looking at
m_next and m_prev which could be modified at the same time from another thread.

[Mads Ager (chromium), 2014-02-17 15:03:43]

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode212
Source/heap/Handle.h:212: template<typename T, typename RootsAccessor>
Should we have a default template argument here?

[Vyacheslav Egorov (Chromium), 2014-02-17 15:52:34]

Thanks for the review.

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode93
Source/heap/Handle.h:93: // No locking requires. Just checks that we are at the
right thread.
On 2014/02/17 15:00:13, Mads Ager (chromium) wrote:
> required

Done.

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode132
Source/heap/Handle.h:132: typename RootsAccessor::Lock lock;
On 2014/02/17 15:00:13, Mads Ager (chromium) wrote:
> Shouldn't you lock before the debug-mode code? The debug mode code is looking
at
> m_next and m_prev which could be modified at the same time from another
thread.

Good catch. Fixed.

https://codereview.chromium.org/169653003/diff/1/Source/heap/Handle.h#newcode212
Source/heap/Handle.h:212: template<typename T, typename RootsAccessor>
On 2014/02/17 15:03:43, Mads Ager (chromium) wrote:
> Should we have a default template argument here?

Forward declaration from Heap.h provides default argument. I will add a comment
to make it apparent.

[Vyacheslav Egorov (Chromium), 2014-02-17 15:52:45]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-02-17 15:52:52]

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

[commit-bot: I haz the power, 2014-02-17 17:13:04]

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

[commit-bot: I haz the power, 2014-02-17 17:13:04]

Retried try job too often on win_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Vyacheslav Egorov (Chromium), 2014-02-17 18:20:15]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-02-17 18:20:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/169653003/210001

[commit-bot: I haz the power, 2014-02-17 21:08:06]

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

[commit-bot: I haz the power, 2014-02-17 21:08:07]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[tkent, 2014-02-17 23:45:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-02-17 23:46:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/169653003/210001

[haraken, 2014-02-18 01:35:12]

LGTM

https://codereview.chromium.org/169653003/diff/210001/Source/heap/ThreadState...
File Source/heap/ThreadState.cpp (right):

https://codereview.chromium.org/169653003/diff/210001/Source/heap/ThreadState...
Source/heap/ThreadState.cpp:367: void ThreadState::visitPersistents(Visitor*
visitor)

I sometimes find it confusing that we have visitPersistents(), visitRoots()
(which visits persistent handles), visitRoots() (which visits all GC roots),
roots(), m_persistents...

Probably we want to rename the methods based on the following rule:

- "roots" means GC roots (including persistent handles, stack etc)
- "persistents" means persistent handles.

For example, we can rename RootsAccessor to PersistentsAccessor etc.

[Vyacheslav Egorov (Chromium), 2014-02-18 02:21:52]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-02-18 02:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/169653003/100003

[commit-bot: I haz the power, 2014-02-18 04:24:31]

Change committed as 167303