New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 671856 link

Starred by 1 user
Status: Assigned
Owner:
keishi@chromium.org
Cc:
█ sigbjo...@opera.com
oilpan-reviews@chromium.org
haraken@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 591606


Sign in to add a comment

Clean up per-thread things after shipping the per-thread heap

Project Member Reported by haraken@chromium.org, Dec 7 2016 
We should move thread-local weak processing, pre-finalization and eager sweeping to the stop-the-world phase.

We should also remove things like:

- Thread termination GC
- The restriction that persistent handles must be 0 when the thread shuts down
- The logic to park threads
- Orphaned pages
- markAsDead
- Safe points

Comment 1 by sigbjo...@opera.com, Dec 7 2016

Blockedon: 591606

Comment 2 by sigbjo...@opera.com, Dec 7 2016

Components: Blink>MemoryAllocator>GarbageCollection
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e4032bc598ae10eeb99036e383ca2f63b73bd1a

commit 3e4032bc598ae10eeb99036e383ca2f63b73bd1a
Author: sigbjornf <sigbjornf@opera.com>
Date: Tue Jan 10 12:02:43 2017

Remove marking visitors' shouldMarkObject().

With per-thread heap handling being fully enabled, checking for any
cross-thread pointer marking (and not doing it), is no longer an issue.

Retire the should-mark predicate.

R=haraken
BUG=671856

Review-Url: https://codereview.chromium.org/2617393004
Cr-Commit-Position: refs/heads/master@{#442554}

[modify] https://crrev.com/3e4032bc598ae10eeb99036e383ca2f63b73bd1a/third_party/WebKit/Source/platform/heap/InlinedGlobalMarkingVisitor.h
[modify] https://crrev.com/3e4032bc598ae10eeb99036e383ca2f63b73bd1a/third_party/WebKit/Source/platform/heap/MarkingVisitor.h
[modify] https://crrev.com/3e4032bc598ae10eeb99036e383ca2f63b73bd1a/third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6422f3b2391675817cc28ed311776067853f5757

commit 6422f3b2391675817cc28ed311776067853f5757
Author: haraken <haraken@chromium.org>
Date: Fri Jan 13 12:45:32 2017

HeapCompact doesn't need to support multiple threads

Now that the per-thread heaps have been shipped, HeapCompact no longer needs to
support multiple threads. This CL removes that code.

BUG=671856

Review-Url: https://codereview.chromium.org/2633463006
Cr-Commit-Position: refs/heads/master@{#443542}

[modify] https://crrev.com/6422f3b2391675817cc28ed311776067853f5757/third_party/WebKit/Source/platform/heap/HeapCompact.cpp
[modify] https://crrev.com/6422f3b2391675817cc28ed311776067853f5757/third_party/WebKit/Source/platform/heap/HeapCompact.h
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b9ec9d0dd42483207e9fab00d11f3df253d18e4

commit 4b9ec9d0dd42483207e9fab00d11f3df253d18e4
Author: haraken <haraken@chromium.org>
Date: Wed Feb 08 11:13:42 2017

Remove markDead() and isDead()

After the per-thread heap was shipped, markDead() is no longer reachable.
This CL removes code that uses the dead bit.

BUG=671856

Review-Url: https://codereview.chromium.org/2679103005
Cr-Commit-Position: refs/heads/master@{#448956}

[modify] https://crrev.com/4b9ec9d0dd42483207e9fab00d11f3df253d18e4/third_party/WebKit/Source/platform/heap/HeapPage.cpp
[modify] https://crrev.com/4b9ec9d0dd42483207e9fab00d11f3df253d18e4/third_party/WebKit/Source/platform/heap/HeapPage.h
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89d59827bf8706769513e1b6b0ad1a113bbfa65d

commit 89d59827bf8706769513e1b6b0ad1a113bbfa65d
Author: haraken <haraken@chromium.org>
Date: Wed Feb 08 19:50:41 2017

Remove a thread-local termination GC

Now that we've shipped the per-thread heap, a thread-local termination GC
should not be needed. We can just use a normal GC to clean up the thread.

BUG=671856

Review-Url: https://codereview.chromium.org/2682933003
Cr-Commit-Position: refs/heads/master@{#449067}

[modify] https://crrev.com/89d59827bf8706769513e1b6b0ad1a113bbfa65d/third_party/WebKit/Source/platform/heap/BlinkGC.h
[modify] https://crrev.com/89d59827bf8706769513e1b6b0ad1a113bbfa65d/third_party/WebKit/Source/platform/heap/ThreadState.cpp
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 9 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/665243239207d7a0e1e73a4179c55f083c5493ca

commit 665243239207d7a0e1e73a4179c55f083c5493ca
Author: haraken <haraken@chromium.org>
Date: Thu Feb 09 06:21:38 2017

Remove orphaned pages from Oilpan

Now that we have shipped the per-thread heap, orphaned pages are not needed.

orphaned pages was needed because 1) a thread termination GC traces only objects
in the thread's heap and 2) there are cross-thread Member pointers. This means
that after finishing the thread termination GC, it's possible that some Member
pointers are still pointing to the thread's heap. Thus we couldn't immediately
release the thread's heap. Hence we had to mark the pages in the heap as orphaned
until a next GC is triggered.

In the per-thread heap world, cross-thread pointers must be CrossThreadPersistents.
Also we have a logic to clear CrossThreadPersistents pointing to the terminating
thread's heap before the thread terminates. This means that it's guaranteed that
there is no cross-thread pointer pointing to the terminating thread's heap
after the thread terminates. Then orphaned pages are not needed.

BUG=671856

Review-Url: https://codereview.chromium.org/2684633004
Cr-Commit-Position: refs/heads/master@{#449225}

[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/GCInfo.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/Heap.h
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/HeapPage.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/HeapPage.h
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/PagePool.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/PagePool.h
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/PersistentNode.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/ThreadState.cpp
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/ThreadState.h
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/TraceTraits.h
[modify] https://crrev.com/665243239207d7a0e1e73a4179c55f083c5493ca/third_party/WebKit/Source/platform/heap/VisitorImpl.h
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 10 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ac19e1710b6bdf828bc721d56c1f7ba8e48ff73

commit 6ac19e1710b6bdf828bc721d56c1f7ba8e48ff73
Author: haraken <haraken@chromium.org>
Date: Fri Feb 10 09:29:45 2017

HeapCompact does not need to look up other threads' states

Now that the per-thread heap is shipped, HeapCompact does not need to
look up if other threads have pointers on their stacks. This CL removes that code.

BUG=671856

Review-Url: https://codereview.chromium.org/2688093002
Cr-Commit-Position: refs/heads/master@{#449572}

[modify] https://crrev.com/6ac19e1710b6bdf828bc721d56c1f7ba8e48ff73/third_party/WebKit/Source/platform/heap/HeapCompact.cpp
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58532772e54feeec7258d42731695f7d183293b8

commit 58532772e54feeec7258d42731695f7d183293b8
Author: haraken <haraken@chromium.org>
Date: Mon Feb 13 08:01:21 2017

Remove unnecessary SafePointScope

Now that the per-thread heap is working, we don't need to have a SafePointScope
that had been used to give a chance to a GCing thread to stop this thread.

BUG=671856

Review-Url: https://codereview.chromium.org/2688383003
Cr-Commit-Position: refs/heads/master@{#449917}

[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/modules/webdatabase/sqlite/SQLiteFileSystem.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/modules/webdatabase/sqlite/SQLiteStatement.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/platform/WaitableEvent.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/Source/web/WebHeap.cpp
[modify] https://crrev.com/58532772e54feeec7258d42731695f7d183293b8/third_party/WebKit/public/web/WebHeap.h
Project Member

Comment 16 by bugdroid1@chromium.org, Feb 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d674502071b65ada9815fdb81b92beeda4add7dc

commit d674502071b65ada9815fdb81b92beeda4add7dc
Author: haraken <haraken@chromium.org>
Date: Mon Feb 13 10:41:58 2017

Thread-local weak processing should just use a global weak callback stacks

Now I don't think there is any reason we have to run the global weak processing
and the thread-local weak processing separately. We can run them at the same
time in the stop-the-world phase.

BUG=671856

Review-Url: https://codereview.chromium.org/2690913002
Cr-Commit-Position: refs/heads/master@{#449926}

[modify] https://crrev.com/d674502071b65ada9815fdb81b92beeda4add7dc/third_party/WebKit/Source/platform/heap/Heap.cpp
Project Member

Comment 17 by bugdroid1@chromium.org, Feb 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ea6c5dc0ae4a701c4ece99c4c1a0959e08f7722

commit 6ea6c5dc0ae4a701c4ece99c4c1a0959e08f7722
Author: haraken <haraken@chromium.org>
Date: Mon Feb 13 22:20:38 2017

Mark the BlinkGC.TimeForStoppingThreads UMA obsolete

The corresponding code was removed.

BUG=671856

Review-Url: https://codereview.chromium.org/2695583002
Cr-Commit-Position: refs/heads/master@{#450116}

[modify] https://crrev.com/6ea6c5dc0ae4a701c4ece99c4c1a0959e08f7722/tools/metrics/histograms/histograms.xml
Project Member

Comment 18 by bugdroid1@chromium.org, Feb 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80d8d295c8f2445ef62ceff89668b15b15ae644d

commit 80d8d295c8f2445ef62ceff89668b15b15ae644d
Author: haraken <haraken@chromium.org>
Date: Tue Feb 14 10:30:52 2017

Remove SafePointAwareMutexLocker

Now that we have the per-thread heaps, we no longer need SafePointAwareMutexLocker
(because a GCing thread doesn't need to stop other threads that may be waiting on the locker).
We can just use MutexLocker.

BUG=671856

Review-Url: https://codereview.chromium.org/2691073004
Cr-Commit-Position: refs/heads/master@{#450314}

[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/modules/webdatabase/Database.cpp
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/Heap.h
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/HeapTest.cpp
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/SafePoint.cpp
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/SafePoint.h
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/ThreadState.cpp
[modify] https://crrev.com/80d8d295c8f2445ef62ceff89668b15b15ae644d/third_party/WebKit/Source/platform/heap/ThreadState.h
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6908e6e6a69e1c5b11a7a2227d65a913208bdb95

commit 6908e6e6a69e1c5b11a7a2227d65a913208bdb95
Author: haraken <haraken@chromium.org>
Date: Tue Feb 14 13:39:27 2017

Remove thread-local weak processing

It is now unused.

BUG=671856

Review-Url: https://codereview.chromium.org/2694173002
Cr-Commit-Position: refs/heads/master@{#450339}

[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/Heap.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/HeapAllocator.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/Persistent.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/ThreadState.cpp
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/ThreadState.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/Visitor.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/platform/heap/VisitorImpl.h
[modify] https://crrev.com/6908e6e6a69e1c5b11a7a2227d65a913208bdb95/third_party/WebKit/Source/wtf/HashTable.h
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df877d86769f52421f838b8b947569aabe80db6e

commit df877d86769f52421f838b8b947569aabe80db6e
Author: haraken <haraken@chromium.org>
Date: Wed Feb 15 11:17:47 2017

Slightly tidy up the usage of SweepForbiddenScope and ScriptForbiddenScope

SweepForbiddenScope and ScriptForbiddenScope are used together.
This CL just makes it clearer in the code base.

BUG=671856

Review-Url: https://codereview.chromium.org/2692043008
Cr-Commit-Position: refs/heads/master@{#450664}

[modify] https://crrev.com/df877d86769f52421f838b8b947569aabe80db6e/third_party/WebKit/Source/platform/heap/ThreadState.cpp
Project Member

Comment 25 by bugdroid1@chromium.org, Feb 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/580f0df2c038eb1e575b169ed6491e9cbe92bc11

commit 580f0df2c038eb1e575b169ed6491e9cbe92bc11
Author: haraken <haraken@chromium.org>
Date: Fri Feb 17 12:46:51 2017

Remove SafePointScopes in HeapTest.cpp

Currently SafePointScope is used for two purposes:

a) Declare that the thread is not operating on the oilpan heap
and let another thread start a GC.

b) Record the stack end and all registers before starting a GC.

Now that we have the per-thread heap, a) is not needed. b) is still needed though.
The SafePointScopes removed in this CL is a).

BUG=671856

Review-Url: https://codereview.chromium.org/2697383003
Cr-Commit-Position: refs/heads/master@{#451289}

[modify] https://crrev.com/580f0df2c038eb1e575b169ed6491e9cbe92bc11/third_party/WebKit/Source/platform/heap/HeapTest.cpp
Project Member

Comment 27 by bugdroid1@chromium.org, Feb 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e516a63e0c656af33c92ce1ecad182fb6626bff

commit 6e516a63e0c656af33c92ce1ecad182fb6626bff
Author: haraken <haraken@chromium.org>
Date: Fri Feb 17 16:03:55 2017

Remove SafePointScope from WorkerThread

Now that an Oilpan GC runs per thread, the SafePointScope should not be needed.

BUG=671856

Review-Url: https://codereview.chromium.org/2689213015
Cr-Commit-Position: refs/heads/master@{#451313}

[modify] https://crrev.com/6e516a63e0c656af33c92ce1ecad182fb6626bff/third_party/WebKit/Source/core/workers/WorkerThread.cpp
Project Member

Comment 29 by bugdroid1@chromium.org, Feb 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/befbcdf4f36ecd0f2cbc58687bdc9c854156c49e

commit befbcdf4f36ecd0f2cbc58687bdc9c854156c49e
Author: sigbjornf <sigbjornf@opera.com>
Date: Wed Feb 22 13:12:32 2017

Remove unnecessary PagePool locks.

With per-thread heap (arenas), there will not be any contention
on adding and removing page pool entries.

R=haraken
BUG=671856

Review-Url: https://codereview.chromium.org/2707193004
Cr-Commit-Position: refs/heads/master@{#452023}

[modify] https://crrev.com/befbcdf4f36ecd0f2cbc58687bdc9c854156c49e/third_party/WebKit/Source/platform/heap/PagePool.cpp
[modify] https://crrev.com/befbcdf4f36ecd0f2cbc58687bdc9c854156c49e/third_party/WebKit/Source/platform/heap/PagePool.h
Project Member

Comment 31 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a06e40ba77e156543163d150fb13a6064eb2e3da

commit a06e40ba77e156543163d150fb13a6064eb2e3da
Author: haraken <haraken@chromium.org>
Date: Mon Feb 27 05:12:08 2017

Mark the BlinkGC.TimeForThreadLocalWeakProcessing UMA obsolete

The code for thread-local weak processing was already removed.

BUG=671856

Review-Url: https://codereview.chromium.org/2695223002
Cr-Commit-Position: refs/heads/master@{#453156}

[modify] https://crrev.com/a06e40ba77e156543163d150fb13a6064eb2e3da/tools/metrics/histograms/histograms.xml
Project Member

Comment 32 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39df8cdf874294a0aef7937c776d683a7081ecbc

commit 39df8cdf874294a0aef7937c776d683a7081ecbc
Author: haraken <haraken@chromium.org>
Date: Mon Feb 27 11:11:35 2017

Rename m_globalWeakCallbacks to m_weakCallbacks

Now that we don't have a concept of thread-local weak processing,
we can just rename global weak processing to weak processing.

BUG=671856

Review-Url: https://codereview.chromium.org/2692923007
Cr-Commit-Position: refs/heads/master@{#453188}

[modify] https://crrev.com/39df8cdf874294a0aef7937c776d683a7081ecbc/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/39df8cdf874294a0aef7937c776d683a7081ecbc/third_party/WebKit/Source/platform/heap/Heap.h
[modify] https://crrev.com/39df8cdf874294a0aef7937c776d683a7081ecbc/third_party/WebKit/Source/platform/heap/ThreadState.cpp

Comment 34 by mlippautz@chromium.org, May 21 2018

Status: Assigned (was: Untriaged)
►

Sign in to add a comment