Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892713003:
Prepare for multiple ThreadHeaps (Closed)
|
Created:
4 years, 8 months ago by keishi Modified:
4 years, 7 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), haraken, blink-reviews-bindings_chromium.org, 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. |
DescriptionPrepare for multiple ThreadHeaps
Convert static methods and members on ThreadHeap to non-static
BUG=591606
Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b
Cr-Commit-Position: refs/heads/master@{#389052}
Committed: https://crrev.com/75da88a74e60c92b4456b06f254aa6919b4dee01
Cr-Commit-Position: refs/heads/master@{#390056}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 35
Patch Set 8 : fixed #Patch Set 9 : #
Total comments: 2
Patch Set 10 : added NO_SANITIZE_THREAD #
Total comments: 4
Patch Set 11 : Added comment #Patch Set 12 : #
Total comments: 2
Patch Set 13 : #Patch Set 14 : Rebase #Messages
Total messages: 73 (35 generated)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/1892713003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/60001
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Prepare for multiple ThreadHeaps BUG= ========== to ========== Prepare for multiple ThreadHeaps BUG=591606 ==========
Description was changed from ========== Prepare for multiple ThreadHeaps BUG=591606 ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
1. Issue 1892713003: Prepare for multiple ThreadHeaps <- https://codereview.chromium.org/1892713003/ 2. Issue 1910753002: Add enablePerThreadHeap flag to ThreadState https://codereview.chromium.org/1910753002/ 3. Issue 1904003002: CrossThreadPersistentRegion will only trace objects belonging to the current GCing ThreadHeap https://codereview.chromium.org/1904003002/ 4. Issue 1909803002: Add per thread heap enabled MarkingVisitorImpl https://codereview.chromium.org/1909803002/ 5. Issue 1909813002: Enable per thread heap for database thread https://codereview.chromium.org/1909813002/
LGTM with comments. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:61: ParkThreadsScope(ThreadState* state) Add explicit. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:285: if (m_threads.isEmpty()) { m_threads.isEmpty() => thread->isMainThread() https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:739: { Add ASSERT(ThreadState::current()->isInGC()). https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:749: { Add ASSERT(ThreadState::current()->isInGC()). https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:774: if (!m_regionTree) Do we need this check? https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:790: ProcessHeap::resetHeapCounters(); This is TODO. We should not reset the process-wide heap counters. Instead we should subtract the values of this thread's heap counters from the process-wide heap counters. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:793: MutexLocker locker(m_threadAttachMutex); This lock wouldn't be needed because we're in GC here. See line 785. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:186: static ThreadHeap* main() { return s_mainThreadHeap; } isMain => isMainThreadHeap main => mainThreadHeap https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h:47: ThreadHeap::main()->pushTraceCallback(const_cast<void*>(objectPointer), callback); This code is performance-sensitive, but ThreadHeap::main() is just looking up a static variable, so it would be ok. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:250: RELEASE_ASSERT(ThreadState::attachedThreads().size() == 1); Can we keep this assert somewhere? https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:338: RELEASE_ASSERT(state->gcState() == ThreadState::NoGCScheduled); Can we keep this assert somewhere? https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:155: s_threadSpecific = new WTF::ThreadSpecific<ThreadState*>(); Can we move this to ThreadState::attachMainThread and remove ThreadState::init? https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: new(s_mainThreadStateStorage) ThreadState(); Nit: Add a space after 'new'. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:199: void ThreadState::attach() attach => attachCurrentThread ? (for consistency with detachCurrentThread) https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:212: void ThreadState::cleanup() cleanup => runTerminationGC https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:214: if (isMainThread()) { This is okay at the moment, but you'll need to replace these isMainThread() checks with something like isMasterThread(). (master thread = a leader thread of the ThreadHeap) Let's replace isMainThread() with isMasterThread() in a follow-up CL. If we enable the per-thread heaps without doing the replacement, it can cause unexpected bugs. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:706: ThreadState* ThreadState::fromObject(const void* object) Worth inlining this method? https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:708: if (!object) I'd remove this check. Instead add ASSERT(object).
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 Link to the patchset: https://codereview.chromium.org/1892713003/#ps140001 (title: "fixed")
https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:61: ParkThreadsScope(ThreadState* state) On 2016/04/21 11:48:25, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:285: if (m_threads.isEmpty()) { On 2016/04/21 11:48:25, haraken wrote: > > m_threads.isEmpty() => thread->isMainThread() Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:739: { On 2016/04/21 11:48:25, haraken wrote: > > Add ASSERT(ThreadState::current()->isInGC()). Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:749: { On 2016/04/21 11:48:25, haraken wrote: > > Add ASSERT(ThreadState::current()->isInGC()). Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:774: if (!m_regionTree) On 2016/04/21 11:48:25, haraken wrote: > > Do we need this check? Removed. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:790: ProcessHeap::resetHeapCounters(); On 2016/04/21 11:48:25, haraken wrote: > > This is TODO. We should not reset the process-wide heap counters. Instead we > should subtract the values of this thread's heap counters from the process-wide > heap counters. > > Sorry. Meant to deal with this later and forgot. Done https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.cpp:793: MutexLocker locker(m_threadAttachMutex); On 2016/04/21 11:48:25, haraken wrote: > > This lock wouldn't be needed because we're in GC here. See line 785. > Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Heap.h:186: static ThreadHeap* main() { return s_mainThreadHeap; } On 2016/04/21 11:48:25, haraken wrote: > > isMain => isMainThreadHeap > main => mainThreadHeap Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h:47: ThreadHeap::main()->pushTraceCallback(const_cast<void*>(objectPointer), callback); On 2016/04/21 11:48:25, haraken wrote: > > This code is performance-sensitive, but ThreadHeap::main() is just looking up a > static variable, so it would be ok. Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:250: RELEASE_ASSERT(ThreadState::attachedThreads().size() == 1); On 2016/04/21 11:48:25, haraken wrote: > > Can we keep this assert somewhere? I moved this assert to ThreadHeap::detach() https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:338: RELEASE_ASSERT(state->gcState() == ThreadState::NoGCScheduled); On 2016/04/21 11:48:25, haraken wrote: > > Can we keep this assert somewhere? Done. Added back to ThreadState::detachCurrentThread https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:155: s_threadSpecific = new WTF::ThreadSpecific<ThreadState*>(); On 2016/04/21 11:48:25, haraken wrote: > > Can we move this to ThreadState::attachMainThread and remove ThreadState::init? Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: new(s_mainThreadStateStorage) ThreadState(); On 2016/04/21 11:48:26, haraken wrote: > > Nit: Add a space after 'new'. Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:199: void ThreadState::attach() On 2016/04/21 11:48:25, haraken wrote: > > attach => attachCurrentThread ? (for consistency with detachCurrentThread) Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:212: void ThreadState::cleanup() On 2016/04/21 11:48:26, haraken wrote: > > cleanup => runTerminationGC Done. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:706: ThreadState* ThreadState::fromObject(const void* object) On 2016/04/21 11:48:25, haraken wrote: > > Worth inlining this method? It's hard to avoid a circular include right now. https://codereview.chromium.org/1892713003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:708: if (!object) On 2016/04/21 11:48:25, haraken wrote: > > I'd remove this check. Instead add ASSERT(object). Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/140001
Message was sent while issue was closed.
Description was changed from ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1919773002/ by keishi@chromium.org. The reason for reverting is: Failed TSan bots.
Message was sent while issue was closed.
Description was changed from ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ==========
keishi@chromium.org changed reviewers: + glider@chromium.org
+glider I think you were right. ThreadState::visitStackRoots was already in the suppression list and I moved that in this CL so I guess I need to update that line. Could you review the change to build/sanitizers/tsan_suppressions.cc?
Not LGTM https://codereview.chromium.org/1892713003/diff/160001/build/sanitizers/tsan_... File build/sanitizers/tsan_suppressions.cc (right): https://codereview.chromium.org/1892713003/diff/160001/build/sanitizers/tsan_... build/sanitizers/tsan_suppressions.cc:278: "race:blink::ThreadState::visitStack\n" Please no. It's better to add the NO_SANITIZE_THREAD attribute to Webkit and use it here.
Like this? https://codereview.chromium.org/1892713003/diff/160001/build/sanitizers/tsan_... File build/sanitizers/tsan_suppressions.cc (right): https://codereview.chromium.org/1892713003/diff/160001/build/sanitizers/tsan_... build/sanitizers/tsan_suppressions.cc:278: "race:blink::ThreadState::visitStack\n" On 2016/04/26 09:03:16, Alexander Potapenko wrote: > Please no. > It's better to add the NO_SANITIZE_THREAD attribute to Webkit and use it here. Done.
Mostly LGTM https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:332: NO_SANITIZE_THREAD Please add a comment here. Something along the lines of "stack scanning may overrun the bounds of local objects and/or race with other threads that use this stack" https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/AddressSanitizer.h (right): https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/AddressSanitizer.h:2: // Use of this source code is governed by a BSD-style license that can be Nit: guess it's time to rename this file, but I don't insist on doing that in this CL.
https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:332: NO_SANITIZE_THREAD On 2016/04/26 10:02:27, Alexander Potapenko wrote: > Please add a comment here. Something along the lines of "stack scanning may > overrun the bounds of local objects and/or race with other threads that use this > stack" Done. https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/AddressSanitizer.h (right): https://codereview.chromium.org/1892713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/AddressSanitizer.h:2: // Use of this source code is governed by a BSD-style license that can be On 2016/04/26 10:02:27, Alexander Potapenko wrote: > Nit: guess it's time to rename this file, but I don't insist on doing that in > this CL. There is already a TODO for this on line 7. I think I'm going to leave this as is for now.
LGTM with a nit https://codereview.chromium.org/1892713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:332: // Stack scanning may overrun the bounds of local objects and/or race with "may overrun the bounds" relates to the NO_SANITIZE_ADDRESS macro, so it's better to move the comment one line above.
Thanks! https://codereview.chromium.org/1892713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1892713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:332: // Stack scanning may overrun the bounds of local objects and/or race with On 2016/04/26 14:04:35, Alexander Potapenko wrote: > "may overrun the bounds" relates to the NO_SANITIZE_ADDRESS macro, so it's > better to move the comment one line above. Done.
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, glider@chromium.org Link to the patchset: https://codereview.chromium.org/1892713003/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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, glider@chromium.org Link to the patchset: https://codereview.chromium.org/1892713003/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892713003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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/1892713003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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/1892713003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892713003/260001
Message was sent while issue was closed.
Description was changed from ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} ========== to ========== Prepare for multiple ThreadHeaps Convert static methods and members on ThreadHeap to non-static BUG=591606 Committed: https://crrev.com/b5cb70c9e931fad94b21fc6d8c9356b7e3860b6b Cr-Commit-Position: refs/heads/master@{#389052} Committed: https://crrev.com/75da88a74e60c92b4456b06f254aa6919b4dee01 Cr-Commit-Position: refs/heads/master@{#390056} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/75da88a74e60c92b4456b06f254aa6919b4dee01 Cr-Commit-Position: refs/heads/master@{#390056} |
