Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 1 month ago by Hannes Payer (out of office) Modified:
3 years, 9 months ago Reviewers:
Michael Lippautz, ulan CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Add atomics to mark bit operations.
BUG=chromium:694255
Review-Url: https://codereview.chromium.org/2492263002
Cr-Commit-Position: refs/heads/master@{#43679}
Committed: https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415df8c0e
Patch Set 1 #Patch Set 2 : [heap] Benchmarking atomic ops. #Patch Set 3 : add include #Patch Set 4 : plain atomics #Patch Set 5 : explicit type #Patch Set 6 : use new atomics based on cast #Patch Set 7 : explicit atomics #Patch Set 8 : nobarrier for get #Patch Set 9 : format #Patch Set 10 : fixup missing call sites #
Total comments: 12
Patch Set 11 : rename #Patch Set 12 : dcheck #
Total comments: 2
Patch Set 13 : comment #
Created: 3 years, 9 months ago
Messages
Total messages: 40 (32 generated)
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16775) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13160)
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [heap] Benchmarking atomic ops. BUG= ========== to ========== [heap] Add atomics to mark bit operations. BUG=chromium:694255 ==========
hpayer@chromium.org changed reviewers: + mlippautz@chromium.org, ulan@chromium.org
This CL only changes the Marking and ObjectMarking operaters to be ATOMIC or NON_ATOMIC. The range functions still need to be changed, separate CL.
https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode = MarkBit::NON_ATOMIC, nit: Can you spell out the names, e.g. modication_mode, marking_mode. https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h File src/heap/marking.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:16: typedef uint32_t CellType; nit: Can we have a static_assert that sizeof(CellType) == sizeof(Atomic32)? https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:18: enum ModificationMode { ATOMIC, NON_ATOMIC }; You could make an enum class ModicationMode outside of MarkBit. We often had problems with enums nested in classes.
https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode = MarkBit::NON_ATOMIC, On 2017/03/06 13:52:54, Michael Lippautz wrote: > nit: Can you spell out the names, e.g. modication_mode, marking_mode. +1, how about calling the parameter AccessMode? https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h File src/heap/marking.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:321: return !mark_bit.Get<mode>() && mark_bit.Next().Get<mode>(); Atomic version can return false positive if between the first bit read and the second bit read the object goes from white to black in concurrent thread. Can we get rid of this function? https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:354: INLINE(static bool MarkWhite(MarkBit markbit)) { All functions that move the color in black -> grey -> white direction are not safe for concurrent usage (e.g. concurrent MarkWhite() and GreyToBlack() on a grey color can make impossible color). Should we disallow atomic versions of MarkWhite and BlackToGrey?
https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.h File src/heap/mark-compact.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode = MarkBit::NON_ATOMIC, On 2017/03/06 13:52:54, Michael Lippautz wrote: > nit: Can you spell out the names, e.g. modication_mode, marking_mode. Done. https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact.... src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode = MarkBit::NON_ATOMIC, On 2017/03/06 13:52:54, Michael Lippautz wrote: > nit: Can you spell out the names, e.g. modication_mode, marking_mode. Done. https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h File src/heap/marking.h (right): https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:16: typedef uint32_t CellType; On 2017/03/06 13:52:54, Michael Lippautz wrote: > nit: Can we have a static_assert that sizeof(CellType) == sizeof(Atomic32)? Done. https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:18: enum ModificationMode { ATOMIC, NON_ATOMIC }; On 2017/03/06 13:52:54, Michael Lippautz wrote: > You could make an > enum class ModicationMode > outside of MarkBit. We often had problems with enums nested in classes. I kind of like it to keep it in the MarkBit class for that particular case. I do not see any problems with that. https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:321: return !mark_bit.Get<mode>() && mark_bit.Next().Get<mode>(); On 2017/03/06 14:51:33, ulan wrote: > Atomic version can return false positive if between the first bit read and the > second bit read the object goes from white to black in concurrent thread. > > Can we get rid of this function? This function is still useful. I made it safe for concurrent usage. https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new... src/heap/marking.h:354: INLINE(static bool MarkWhite(MarkBit markbit)) { On 2017/03/06 14:51:33, ulan wrote: > All functions that move the color in black -> grey -> white direction are not > safe for concurrent usage (e.g. concurrent MarkWhite() and GreyToBlack() on a > grey color can make impossible color). > > Should we disallow atomic versions of MarkWhite and BlackToGrey? Done. Why is BlackToGrey a problem?
lgtm https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h File src/heap/marking.h (right): https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h#new... src/heap/marking.h:384: DCHECK(mode == MarkBit::ATOMIC || IsBlack(markbit)); STATIC_ASSERT(mode == MarkBit::NON_ATOMIC)
https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h File src/heap/marking.h (right): https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h#new... src/heap/marking.h:384: DCHECK(mode == MarkBit::ATOMIC || IsBlack(markbit)); On 2017/03/08 14:42:01, ulan wrote: > STATIC_ASSERT(mode == MarkBit::NON_ATOMIC) Done.
The CQ bit was checked by hpayer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2492263002/#ps230001 (title: "comment")
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": 230001, "attempt_start_ts": 1488990569714430,
"parent_rev": "e7be85daf69bbf9adef4841eae2839e4075f40f7", "commit_rev":
"b643bb771303d33d1ab80577cc58106415df8c0e"}
Message was sent while issue was closed.
Description was changed from ========== [heap] Add atomics to mark bit operations. BUG=chromium:694255 ========== to ========== [heap] Add atomics to mark bit operations. BUG=chromium:694255 Review-Url: https://codereview.chromium.org/2492263002 Cr-Commit-Position: refs/heads/master@{#43679} Committed: https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415d... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
