Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(129)

Issue 1138663002: Oilpan: support eager finalization/sweeping. (Closed)

Created:
5 years, 7 months ago by sof
Modified:
5 years, 7 months ago
Reviewers:
haraken
CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium)
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Oilpan: support eager finalization/sweeping. To accommodate objects that must be finalized promptly, add a designated heap for these. If lazy sweeping is enabled, that heap will be swept eagerly and not delayed until (say) an out-of-line allocation trigger the sweep. If lazy sweeping is disabled, the eagerly swept heap will be processed just like any other. Prompt finalization is needed for objects that can be accessed through "off-heap" references, like direct access from the embedder or through a timer, or some other non-Oilpan object in Blink(*). References that have been considered safe by carefully working through finalization order and other lifetime dependencies. If such 'safe' lifetime dependencies rely on the finalization of other heap objects, they cannot be assumed to hold iff lazy sweeping is enabled. That's because all dead objects are then not finalized synchronously, as observed by the rest of Blink, leading to the possibility that 'off-heap' access of a heap object might access an object that has yet to be finalized (but is known to be dead), or that some of the heap objects it refers to may have finalized by now. Regardless of exact state, an object that shouldn't be accessed in this near-dead state by another party. Hence the introduction of eagerly swept heaps -- all objects residing there that have been deemed unreachable by the marking phase are eagerly swept & finalized before the GC returns. Class hierarchies are marked as eagerly swept by using the WILL_BE_EAGERLY_SWEPT(Class) macro. A very relevant follow-on question here is if we can reliably identify objects that must be eagerly swept. * - while the goal is to remove or otherwise bring all such off-heap references under control within Blink, they currently exist. But not profusely many, but more than we would like to while we're in in a transition state where Oilpan isn't always enabled. R=haraken BUG=420515 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195618

Patch Set 1 #

Total comments: 7

Patch Set 2 : rebase + introduce OILPAN_EAGERLY_SWEEP() #

Total comments: 12

Patch Set 3 : review iteration + rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -17 lines) Patch
M Source/platform/heap/Heap.h View 1 2 3 chunks +45 lines, -17 lines 0 comments Download
M Source/platform/heap/HeapTest.cpp View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
M Source/platform/heap/ThreadState.h View 2 chunks +3 lines, -0 lines 0 comments Download
M Source/platform/heap/ThreadState.cpp View 1 2 2 chunks +24 lines, -0 lines 0 comments Download

Messages

Total messages: 34 (2 generated)
haraken
Thanks for the patch! The implementation looks correct. Here are a couple of comments. Currently ...
5 years, 7 months ago (2015-05-11 01:20:27 UTC) #2
sof
On 2015/05/11 01:20:27, haraken wrote: > Thanks for the patch! The implementation looks correct. > ...
5 years, 7 months ago (2015-05-11 05:20:13 UTC) #3
sof
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h#newcode1158 Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE) \ On 2015/05/11 01:20:27, haraken wrote: > ...
5 years, 7 months ago (2015-05-11 05:20:22 UTC) #4
haraken
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h#newcode1158 Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE) \ On 2015/05/11 05:20:22, sof wrote: > ...
5 years, 7 months ago (2015-05-11 06:44:08 UTC) #5
haraken
> > Currently we have three similar mechanisms to avoid problems related to the > ...
5 years, 7 months ago (2015-05-11 06:50:11 UTC) #6
sof
On 2015/05/11 06:50:11, haraken wrote: > > > Currently we have three similar mechanisms to ...
5 years, 7 months ago (2015-05-11 06:52:31 UTC) #7
haraken
On 2015/05/11 06:52:31, sof wrote: > On 2015/05/11 06:50:11, haraken wrote: > > > > ...
5 years, 7 months ago (2015-05-11 08:10:26 UTC) #8
sof
On 2015/05/11 08:10:26, haraken wrote: > On 2015/05/11 06:52:31, sof wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 08:46:13 UTC) #9
haraken
On 2015/05/11 08:46:13, sof wrote: > On 2015/05/11 08:10:26, haraken wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 09:14:52 UTC) #10
sof
On 2015/05/11 09:14:52, haraken wrote: > ... > > Except for Timers and LifecycleObservers, how ...
5 years, 7 months ago (2015-05-11 09:31:31 UTC) #11
haraken
On 2015/05/11 09:31:31, sof wrote: > On 2015/05/11 09:14:52, haraken wrote: > > > ...
5 years, 7 months ago (2015-05-11 09:36:11 UTC) #12
haraken
On 2015/05/11 09:36:11, haraken wrote: > On 2015/05/11 09:31:31, sof wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 09:47:12 UTC) #13
haraken
On 2015/05/11 09:47:12, haraken wrote: > On 2015/05/11 09:36:11, haraken wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 09:50:22 UTC) #14
sof
On 2015/05/11 09:50:22, haraken wrote: > ... > > The next question: Why does the ...
5 years, 7 months ago (2015-05-11 10:16:09 UTC) #15
haraken
On 2015/05/11 10:16:09, sof wrote: > On 2015/05/11 09:50:22, haraken wrote: > > > ...
5 years, 7 months ago (2015-05-11 13:09:41 UTC) #16
sof
On 2015/05/11 13:09:41, haraken wrote: > On 2015/05/11 10:16:09, sof wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 13:22:51 UTC) #17
haraken
On 2015/05/11 13:22:51, sof wrote: > On 2015/05/11 13:09:41, haraken wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-11 13:32:16 UTC) #18
haraken
On 2015/05/11 09:31:31, sof wrote: > On 2015/05/11 09:14:52, haraken wrote: > > > ...
5 years, 7 months ago (2015-05-18 05:12:42 UTC) #19
sof
On 2015/05/18 05:12:42, haraken wrote: > On 2015/05/11 09:31:31, sof wrote: > > On 2015/05/11 ...
5 years, 7 months ago (2015-05-18 05:22:14 UTC) #20
haraken
> > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an issue > > ...
5 years, 7 months ago (2015-05-18 15:12:17 UTC) #21
sof
On 2015/05/18 15:12:17, haraken wrote: > > > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out ...
5 years, 7 months ago (2015-05-18 20:22:43 UTC) #22
haraken
On 2015/05/18 20:22:43, sof wrote: > On 2015/05/18 15:12:17, haraken wrote: > > > > ...
5 years, 7 months ago (2015-05-18 23:18:03 UTC) #23
haraken
On 2015/05/18 23:18:03, haraken wrote: > On 2015/05/18 20:22:43, sof wrote: > > On 2015/05/18 ...
5 years, 7 months ago (2015-05-19 10:35:58 UTC) #24
sof
On 2015/05/19 10:35:58, haraken wrote: > On 2015/05/18 23:18:03, haraken wrote: > > On 2015/05/18 ...
5 years, 7 months ago (2015-05-19 14:59:56 UTC) #25
sof
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h#newcode1158 Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE) \ On 2015/05/11 06:44:07, haraken wrote: > ...
5 years, 7 months ago (2015-05-19 15:00:15 UTC) #26
haraken
LGTM Just to confirm: We're are going to add the EAGERLY_SWEEP macro only on LifecycleObserver, ...
5 years, 7 months ago (2015-05-19 23:23:38 UTC) #27
sof
https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Heap.h#newcode1103 Source/platform/heap/Heap.h:1103: return NormalPage2HeapIndex; On 2015/05/19 23:23:37, haraken wrote: > > ...
5 years, 7 months ago (2015-05-20 09:43:01 UTC) #28
sof
On 2015/05/19 23:23:38, haraken wrote: > LGTM > > Just to confirm: We're are going ...
5 years, 7 months ago (2015-05-20 09:46:50 UTC) #29
haraken
Still LGTM
5 years, 7 months ago (2015-05-20 12:30:10 UTC) #30
sof
Adding EAGERLY_SWEEP() annotations in a follow-up CL.
5 years, 7 months ago (2015-05-20 12:42:53 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138663002/40001
5 years, 7 months ago (2015-05-20 12:43:16 UTC) #33
commit-bot: I haz the power
5 years, 7 months ago (2015-05-20 14:16:42 UTC) #34
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195618

Powered by Google App Engine
This is Rietveld 408576698