Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(613)

Issue 2007283002: Allow CrossThreadPersistent in collections (Closed)

Created:
4 years, 4 months ago by keishi
Modified:
4 years, 3 months ago
Reviewers:
oilpan-reviews, haraken, sof
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.

Description

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}

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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+313 lines, -3 lines) Patch
M third_party/WebKit/Source/platform/heap/HeapAllocator.h View 1 2 3 4 5 6 7 8 3 chunks +39 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/HeapTest.cpp View 1 2 3 4 5 6 1 chunk +238 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/Persistent.h View 1 2 3 4 5 6 6 chunks +34 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/PersistentNode.cpp View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 32 (9 generated)
keishi
PTAL Part of: 1. Add checks for per thread heap https://codereview.chromium.org/2012763002/ 2. This. Allow CrossThreadPersistent ...
4 years, 4 months ago (2016-05-25 13:08:49 UTC) #2
sof
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/Persistent.h File third_party/WebKit/Source/platform/heap/Persistent.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/Persistent.h#newcode213 third_party/WebKit/Source/platform/heap/Persistent.h:213: if (!m_persistentNode) Moving this up here (again) will lead ...
4 years, 4 months ago (2016-05-25 13:11:42 UTC) #4
sof
Some unit tests would be good. https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425 third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> ...
4 years, 4 months ago (2016-05-25 13:26:39 UTC) #5
keishi
Added HeapTest.CrossThreadPersistent{Vector,Set} https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425 third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> ...
4 years, 4 months ago (2016-05-26 04:23:14 UTC) #6
keishi
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425 third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> { On ...
4 years, 4 months ago (2016-05-26 04:45:43 UTC) #7
sof
https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425 third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> { On ...
4 years, 4 months ago (2016-05-26 05:14:05 UTC) #8
keishi
> That would my preference. Done. Changed to PersistentBase.
4 years, 4 months ago (2016-05-26 09:12:32 UTC) #9
sof
https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode565 third_party/WebKit/Source/platform/heap/HeapAllocator.h:565: template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>> : SimpleClassHashTraits<blink::CrossThreadPersistent<T>> { Can we ...
4 years, 4 months ago (2016-05-26 09:41:44 UTC) #10
keishi
https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode565 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 ...
4 years, 4 months ago (2016-05-26 11:38:17 UTC) #11
sof
getting close. https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h#newcode206 third_party/WebKit/Source/platform/heap/Heap.h:206: if (!ThreadState::current()) What's going wrong without this ...
4 years, 4 months ago (2016-05-26 12:09:41 UTC) #12
keishi
https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h#newcode206 third_party/WebKit/Source/platform/heap/Heap.h:206: if (!ThreadState::current()) On 2016/05/26 12:09:41, sof wrote: > What's ...
4 years, 4 months ago (2016-05-26 12:32:04 UTC) #13
sof
lgtm, thanks for the iterations.
4 years, 3 months ago (2016-05-26 18:13:57 UTC) #14
haraken
LGTM
4 years, 3 months ago (2016-05-26 18:15:52 UTC) #15
commit-bot: I haz the power
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
4 years, 3 months ago (2016-05-27 04:32:01 UTC) #17
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-05-27 06:44:01 UTC) #18
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d Cr-Commit-Position: refs/heads/master@{#396410}
4 years, 3 months ago (2016-05-27 06:45:56 UTC) #20
keishi
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2028433003/ by keishi@chromium.org. ...
4 years, 3 months ago (2016-05-31 01:22:11 UTC) #21
commit-bot: I haz the power
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
4 years, 3 months ago (2016-06-01 05:45:55 UTC) #25
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 3 months ago (2016-06-01 07:24:16 UTC) #27
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/365962a919dbc711a23bc388c214778367522ba3 Cr-Commit-Position: refs/heads/master@{#397076}
4 years, 3 months ago (2016-06-01 07:26:56 UTC) #29
sof
what did the perf regression uncover?
4 years, 3 months ago (2016-06-01 08:00:01 UTC) #30
keishi
On 2016/06/01 08:00:01, sof wrote: > what did the perf regression uncover? I'm not sure ...
4 years, 3 months ago (2016-06-01 08:09:05 UTC) #31
keishi
4 years, 3 months ago (2016-06-02 01:35:47 UTC) #32
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...

Powered by Google App Engine
This is Rietveld 408576698