Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547853002:
Add section for heap collections to BlinkGC documentation (Closed)
DescriptionAdd section for heap collections to BlinkGC documentation
BUG=544756
Committed: https://crrev.com/d4658a2a9ed1fdaa2729cb4fc9101b92f488907c
Cr-Commit-Position: refs/heads/master@{#436227}
Patch Set 1 #
Total comments: 6
Patch Set 2 : added sample code #Messages
Total messages: 13 (7 generated)
Description was changed from ========== Add section for heap collections to BlinkGC documentation BUG= ========== to ========== Add section for heap collections to BlinkGC documentation BUG=544756 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
PTAL
LGTM https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md (right): https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:444: Heap collections are special in that the types themselves do not inherit from GarbageCollected (hence they are not allocated on the Oilpan heap) but they still *need to be traced* from the trace method (because we need to trace the backing store which is on the Oilpan heap). Shall we add some code example? https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:446: When you want to add a heap collection as a member of a non-garbage-collected class, please use the persistent variants (just prefix the type with Persistent e.g. PersistentHeapVector, PersistentHeapHashMap, etc.). Ditto. https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:448: Please be very cautious if you want to use a heap collection from multiple threads. Reference to heap collections may be passed to another thread using CrossThreadPersistents, but *you may not modify the collection from the non-owner thread*. This is because modifications to collections may trigger backing store reallocations, and Oilpan's per thread heap requires that modifications to a heap happen on its owner thread. I'm just curious but do we have a runtime verification to detect the multi-thread access?
https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md (right): https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:444: Heap collections are special in that the types themselves do not inherit from GarbageCollected (hence they are not allocated on the Oilpan heap) but they still *need to be traced* from the trace method (because we need to trace the backing store which is on the Oilpan heap). On 2016/12/02 12:50:46, haraken wrote: > > Shall we add some code example? Done. https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:446: When you want to add a heap collection as a member of a non-garbage-collected class, please use the persistent variants (just prefix the type with Persistent e.g. PersistentHeapVector, PersistentHeapHashMap, etc.). On 2016/12/02 12:50:46, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2547853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:448: Please be very cautious if you want to use a heap collection from multiple threads. Reference to heap collections may be passed to another thread using CrossThreadPersistents, but *you may not modify the collection from the non-owner thread*. This is because modifications to collections may trigger backing store reallocations, and Oilpan's per thread heap requires that modifications to a heap happen on its owner thread. On 2016/12/02 12:50:46, haraken wrote: > > I'm just curious but do we have a runtime verification to detect the > multi-thread access? We don't check for every modification (i.e. the verification in Member::checkPointer won't work if Member was initialized with memset), but we do check on reallocation (backingExpand/backingShrink).
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/2547853002/#ps20001 (title: "added sample code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480902953961800,
"parent_rev": "4cad0256e204ad2158d74f853b4257d19c5b5b4e", "commit_rev":
"32cff9d9d6f0d04a3b0d2343b67da248597f6b9a"}
Message was sent while issue was closed.
Description was changed from ========== Add section for heap collections to BlinkGC documentation BUG=544756 ========== to ========== Add section for heap collections to BlinkGC documentation BUG=544756 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add section for heap collections to BlinkGC documentation BUG=544756 ========== to ========== Add section for heap collections to BlinkGC documentation BUG=544756 Committed: https://crrev.com/d4658a2a9ed1fdaa2729cb4fc9101b92f488907c Cr-Commit-Position: refs/heads/master@{#436227} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d4658a2a9ed1fdaa2729cb4fc9101b92f488907c Cr-Commit-Position: refs/heads/master@{#436227} |
