Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007283002:
Allow CrossThreadPersistent in collections (Closed)
|
Created:
4 years, 7 months ago by keishi Modified:
4 years, 6 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews, kinuko+watch, kouhei+heap_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow CrossThreadPersistent in collections
BUG=591606
Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d
Cr-Commit-Position: refs/heads/master@{#396410}
Committed: https://crrev.com/365962a919dbc711a23bc388c214778367522ba3
Cr-Commit-Position: refs/heads/master@{#397076}
Patch Set 1 #
Total comments: 13
Patch Set 2 : #Patch Set 3 : fix #Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #Patch Set 8 : Reduce usage of HandleHashTrait #Patch Set 9 : #
Messages
Total messages: 32 (9 generated)
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
PTAL Part of: 1. Add checks for per thread heap https://codereview.chromium.org/2012763002/ 2. This. Allow CrossThreadPersistent in collections https://codereview.chromium.org/2007283002/ 3. Enable per thread heap for database thread https://codereview.chromium.org/1909813002/
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/Persistent.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/Persistent.h:213: if (!m_persistentNode) Moving this up here (again) will lead to a race.
Some unit tests would be good. https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> { Why is this needed for CrossThreadPersistent, but not Persistent? Or to put it differently, why not add support for both kinds at the same time? https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:428: static const bool canInitializeWithMemset = false; This can be |true|. https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:430: static const bool canMoveWithMemcpy = false; Wouldn't |true| work out? https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:564: // FIXME: The distinction between PeekInType and PassInType is there for nit: shouldn't be adding new FIXMEs by now. https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:571: using IteratorGetType = blink::Member<T>*; why Member<> ?
Added HeapTest.CrossThreadPersistent{Vector,Set}
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename
T> struct VectorTraits<blink::CrossThreadPersistent<T>> :
VectorTraitsBase<blink::CrossThreadPersistent<T>> {
On 2016/05/25 13:26:39, sof wrote:
> Why is this needed for CrossThreadPersistent, but not Persistent? Or to put it
> differently, why not add support for both kinds at the same time?
I need this to do
typedef Deque<CrossThreadPersistent<SQLTransactionBackend>> TransactionsQueue;
in SQLTransactionCoordinator.h
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:428: static const bool
canInitializeWithMemset = false;
On 2016/05/25 13:26:39, sof wrote:
> This can be |true|.
Done.
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:430: static const bool
canMoveWithMemcpy = false;
On 2016/05/25 13:26:39, sof wrote:
> Wouldn't |true| work out?
Done.
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:564: // FIXME: The
distinction between PeekInType and PassInType is there for
On 2016/05/25 13:26:39, sof wrote:
> nit: shouldn't be adding new FIXMEs by now.
Done.
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:571: using
IteratorGetType = blink::Member<T>*;
On 2016/05/25 13:26:39, sof wrote:
> why Member<> ?
Done.
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> { On 2016/05/26 04:23:13, keishi wrote: > On 2016/05/25 13:26:39, sof wrote: > > Why is this needed for CrossThreadPersistent, but not Persistent? Or to put it > > differently, why not add support for both kinds at the same time? > > I need this to do > typedef Deque<CrossThreadPersistent<SQLTransactionBackend>> TransactionsQueue; > > in SQLTransactionCoordinator.h So I don't need support for Persistent right now. Should I still add it?
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> { On 2016/05/26 04:45:43, keishi wrote: > On 2016/05/26 04:23:13, keishi wrote: > > On 2016/05/25 13:26:39, sof wrote: > > > Why is this needed for CrossThreadPersistent, but not Persistent? Or to put > it > > > differently, why not add support for both kinds at the same time? > > > > I need this to do > > typedef Deque<CrossThreadPersistent<SQLTransactionBackend>> TransactionsQueue; > > > > in SQLTransactionCoordinator.h > > So I don't need support for Persistent right now. Should I still add it? That would my preference.
> That would my preference. Done. Changed to PersistentBase.
https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:565: template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>> : SimpleClassHashTraits<blink::CrossThreadPersistent<T>> { Can we make this be over PersistentBase<> also, or do we need to split it out into two specializations? https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Persistent.h (right): https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Persistent.h:383: CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { } Isn't this ctor needed for Persistent<> also? https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Persistent.h:695: struct CrossThreadPersistentHash : MemberHash<T> { Add for Persistent<> also.
https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:565: template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>> : SimpleClassHashTraits<blink::CrossThreadPersistent<T>> { On 2016/05/26 09:41:44, sof wrote: > Can we make this be over PersistentBase<> also, or do we need to split it out > into two specializations? I tried PersistentBase but it didn't work so had to split it. But I created HandleHashTrait to share the code.
getting close. https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:206: if (!ThreadState::current()) What's going wrong without this bail out? I thought the predicate would just inspect the mark bit in the header preceding |object|. https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/HeapTest.cpp:3034: TEST(HeapTest, CrossThreadPersistentSet) Could you create the Persistent edition of this test also? https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Persistent.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Persistent.h:383: CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { } The Persistent<> version of this?
https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:206: if (!ThreadState::current()) On 2016/05/26 12:09:41, sof wrote: > What's going wrong without this bail out? I thought the predicate would just > inspect the mark bit in the header preceding |object|. Sorry this is part of https://codereview.chromium.org/2012763002/ . I used the wrong diff base. I explain in the comments there, but the tests below seem to be creating CrossThreadPersistents on non-oilpan attached threads. > ExtensionApiTest.TabAudible > ScriptStreamingTest.EncodingFromBOM > ScriptStreamingTest.EncodingChanges > ScriptStreamingTest.SuppressingStreaming > ScriptStreamingTest.CompilingStreamedScriptWithParseError > ScriptStreamingTest.ScriptsWithSmallFirstChunk > ScriptStreamingTest.CancellingStreaming > ScriptStreamingTest.CompilingStreamedScript https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Persistent.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Persistent.h:383: CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { } On 2016/05/26 12:09:41, sof wrote: > The Persistent<> version of this? Done.
lgtm, thanks for the iterations.
LGTM
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007283002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007283002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Allow CrossThreadPersistent in collections BUG=591606 ========== to ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2028433003/ by keishi@chromium.org. The reason for reverting is: blink_perf.bindings regression on android.webview crbug.com/615832.
Message was sent while issue was closed.
Description was changed from ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ========== to ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ==========
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2007283002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007283002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007283002/160001
Message was sent while issue was closed.
Description was changed from ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ========== to ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} ========== to ========== Allow CrossThreadPersistent in collections BUG=591606 Committed: https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410} Committed: https://crrev.com/365962a919dbc711a23bc388c214778367522ba3 Cr-Commit-Position: refs/heads/master@{#397076} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/365962a919dbc711a23bc388c214778367522ba3 Cr-Commit-Position: refs/heads/master@{#397076}
Message was sent while issue was closed.
what did the perf regression uncover?
Message was sent while issue was closed.
On 2016/06/01 08:00:01, sof wrote: > what did the perf regression uncover? I'm not sure about the cause since it was only observed on one test on one bot (android-webview-nexus5X). But because the test is doesn't use threads I guessed that the change to HashTraits<blink::Member<T>> somehow effected it. PS9 reverts that part and I'm seeing if it fixes it.
Message was sent while issue was closed.
On 2016/06/01 08:09:05, keishi wrote: > On 2016/06/01 08:00:01, sof wrote: > > what did the perf regression uncover? > > I'm not sure about the cause since it was only observed on one test on one bot > (android-webview-nexus5X). But because the test is doesn't use threads I guessed > that the change to HashTraits<blink::Member<T>> somehow effected it. PS9 reverts > that part and I'm seeing if it fixes it. Looks like it worked. Regression not seen after relanded. https://chromeperf.appspot.com/report?sid=c9da549580958add3d6b6fb5f1a3ad6bc51... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
