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

Issue 2041413005: VM: Fix WeakProperty processing during parallel marking. (Closed)

Created:
4 years, 6 months ago by Vyacheslav Egorov (Google)
Modified:
4 years, 6 months ago
Reviewers:
siva
CC:
reviews_dartlang.org, vm-dev_dartlang.org, Ivan Posva
Base URL:
git@github.com:dart-lang/sdk.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

VM: Fix WeakProperty processing during parallel marking. Previously if one marker marked the weak property and another marker marked its key then the first marker might stop marking before it sees that the key was marked. Here is a possible concurrent execution, assuming P is a WeakProperty and K is P.key: (Marker A) (Marker B) | | [ mark property P ] | | | [ drain marking stack ] | [ no more work to do ] | | [ mark K ] | [ draing marking stack ] | [ no more work to do ] | | | | ... ... | | [Finalize] [Finalize] | [Clear P] In this execution we end up clearing P even though P.key is marked. To fix this issue without reintroducing central WeakProperty processing we change the marking phase loop in such a way that markers consider the marking done only if all of them agree that they have no more weak properties with marked keys. Essentially the marking loop now has a separate phase where all markers check their pending weak properties. The decision to proceed to the next marking phase (weak handles processing) is done in lock step using barrier - either all threads advance to the next stage or one of the threads finds a weak property with marked key and unmarked value and all threads resume marking. BUG= R=asiva@google.com Committed: https://github.com/dart-lang/sdk/commit/97a8c4caffd2e758c60a3e4502f07d6886ee0363

Patch Set 1 #

Patch Set 2 : restore assertion in the right place #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -44 lines) Patch
M runtime/platform/assert.h View 1 chunk +6 lines, -0 lines 0 comments Download
M runtime/vm/gc_marker.cc View 1 5 chunks +94 lines, -44 lines 9 comments Download

Messages

Total messages: 9 (3 generated)
Vyacheslav Egorov (Google)
PTAL
4 years, 6 months ago (2016-06-08 16:25:29 UTC) #2
Vyacheslav Egorov (Google)
+iposva fyi
4 years, 6 months ago (2016-06-08 16:28:43 UTC) #4
siva
lgtm with some questions. https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc File runtime/vm/gc_marker.cc (right): https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc#newcode630 runtime/vm/gc_marker.cc:630: barrier_->Sync(); Why is this barrier_->Sync() ...
4 years, 6 months ago (2016-06-08 19:45:21 UTC) #5
Vyacheslav Egorov (Google)
https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc File runtime/vm/gc_marker.cc (right): https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc#newcode630 runtime/vm/gc_marker.cc:630: barrier_->Sync(); On 2016/06/08 19:45:20, siva wrote: > Why is ...
4 years, 6 months ago (2016-06-09 08:17:06 UTC) #6
siva
LGTM https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc File runtime/vm/gc_marker.cc (right): https://codereview.chromium.org/2041413005/diff/20001/runtime/vm/gc_marker.cc#newcode630 runtime/vm/gc_marker.cc:630: barrier_->Sync(); On 2016/06/09 08:17:05, Vyacheslav Egorov (Google) wrote: ...
4 years, 6 months ago (2016-06-09 16:39:53 UTC) #7
Vyacheslav Egorov (Google)
4 years, 6 months ago (2016-06-09 16:50:26 UTC) #9
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as
97a8c4caffd2e758c60a3e4502f07d6886ee0363 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698