Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1167863002:
Remove unused locking functionality from base::SharedMemory. (Closed)
|
Created:
5 years, 6 months ago by erikchen Modified:
5 years, 6 months ago Reviewers:
Nico CC:
chromium-reviews, erikwright+watch_chromium.org, gavinp+memory_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@shared_memory_remove_abtest Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unused locking functionality from base::SharedMemory.
The only existing consumers of the locking functionality were two unit tests for
SharedMemory.
The first unit test only tested the locking functionality across threads, so I
removed the test.
The second unit test was intended to test cross-process functionality of
base::SharedMemory, and used the locking functionality as a synchronization
method between processes. I rewrote the test to actually test Shared Memory
functionality, as the original test only tested synchronization between
processes, and would have succeeded even if Shared Memory didn't work. I
replaced the synchronization with compare and swap operations.
BUG=466437, 345734
Committed: https://crrev.com/893dadc6e82a64c8135b6f4852ee0960ea62e4a5
Cr-Commit-Position: refs/heads/master@{#335135}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : Rebase against top of tree. #Patch Set 6 : Comments from thakis. #Patch Set 7 : Comments from thakis, round two. #Patch Set 8 : Use a barrier FAA. #Patch Set 9 : Rebase against top of tree. #Patch Set 10 : Disable test on Android. #Patch Set 11 : Remove constant from android translation unit. #
Messages
Total messages: 41 (13 generated)
erikchen@chromium.org changed reviewers: + thakis@chromium.org
thakis: Please review.
On 2015/06/03 23:45:37, erikchen wrote: > thakis: Please review. ping?
On 2015/06/06 01:21:48, erikchen wrote: > On 2015/06/03 23:45:37, erikchen wrote: > > thakis: Please review. > > ping? Should I find a different review for base/?
erikchen@chromium.org changed reviewers: + danakj@chromium.org
danakj: Please review.
https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.h (left): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory.h:315: ino_t inode_; didn't you delete this field yesterday? i guess this just isn't rebased yet? https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:604: int old_value = base::subtle::NoBarrier_CompareAndSwap(ptr, j, j + 1); can you use a lock instead? https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:613: static const uint32 data_size_; s_data_size for consistency
erikchen@chromium.org changed reviewers: - danakj@chromium.org
-danakj
thakis: PTAL https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.h (left): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory.h:315: ino_t inode_; On 2015/06/11 15:39:18, Nico wrote: > didn't you delete this field yesterday? i guess this just isn't rebased yet? Rebased the CL. https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:604: int old_value = base::subtle::NoBarrier_CompareAndSwap(ptr, j, j + 1); On 2015/06/11 15:39:18, Nico wrote: > can you use a lock instead? This code needs to be atomic across multiple processes. The old implementation grabbed a file lock, which for this test would be both slower and less generic (since I'm changing shared memory to not necessarily be backed by a file). https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:613: static const uint32 data_size_; On 2015/06/11 15:39:18, Nico wrote: > s_data_size for consistency Done.
https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:604: int old_value = base::subtle::NoBarrier_CompareAndSwap(ptr, j, j + 1); On 2015/06/11 17:36:30, erikchen wrote: > On 2015/06/11 15:39:18, Nico wrote: > > can you use a lock instead? > > This code needs to be atomic across multiple processes. The old implementation > grabbed a file lock, which for this test would be both slower and less generic > (since I'm changing shared memory to not necessarily be backed by a file). Ah, right. Maybe say "// This runs concurrently in multiple processes, writes need to be atomic." at the top of the loop. Since this is doing an increment, how about using NoBarrier_AtomicIncrement()?
https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_unittest.cc (right): https://codereview.chromium.org/1167863002/diff/60001/base/memory/shared_memo... base/memory/shared_memory_unittest.cc:604: int old_value = base::subtle::NoBarrier_CompareAndSwap(ptr, j, j + 1); On 2015/06/11 17:46:40, Nico wrote: > On 2015/06/11 17:36:30, erikchen wrote: > > On 2015/06/11 15:39:18, Nico wrote: > > > can you use a lock instead? > > > > This code needs to be atomic across multiple processes. The old implementation > > grabbed a file lock, which for this test would be both slower and less generic > > (since I'm changing shared memory to not necessarily be backed by a file). > > Ah, right. Maybe say "// This runs concurrently in multiple processes, writes > need to be atomic." at the top of the loop. > > Since this is doing an increment, how about using NoBarrier_AtomicIncrement()? Done. That's weird. I thought I did use an atomic increment. I must have discovered atomicops.h, started reading through that, come across CAS first, and said, "o hey that will work", forgetting all about FAA.
lgtm!
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167863002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/06/11 20:57:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) thakis: Not sure why this would fail on Android, given that the previous code worked. [ RUN ] SharedMemoryProcessTest.SharedMemoryAcrossProcesses ../../base/memory/shared_memory_unittest.cc:642: Failure Value of: *ptr Actual: 0 Expected: kNumTasks Which is: 5
On 2015/06/11 21:10:08, erikchen wrote: > On 2015/06/11 20:57:02, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > thakis: Not sure why this would fail on Android, given that the previous code > worked. > > [ RUN ] SharedMemoryProcessTest.SharedMemoryAcrossProcesses > ../../base/memory/shared_memory_unittest.cc:642: Failure > Value of: *ptr > Actual: 0 > > Expected: kNumTasks > Which is: 5 ARM has a much weaker memory model than x86 (search for "arm memory model", e.g. http://events.linuxfoundation.org/sites/events/files/slides/weak-to-weedy.pdf). The previous code used a lock, which guarantees all kinds of things – working with atomics is subtle (hence the name). The Barrier version probably does the trick, but that's why we prefer locks over atomics.
On 2015/06/11 21:40:34, Nico wrote: > On 2015/06/11 21:10:08, erikchen wrote: > > On 2015/06/11 20:57:02, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > thakis: Not sure why this would fail on Android, given that the previous code > > worked. > > > > [ RUN ] SharedMemoryProcessTest.SharedMemoryAcrossProcesses > > ../../base/memory/shared_memory_unittest.cc:642: Failure > > Value of: *ptr > > Actual: 0 > > > > Expected: kNumTasks > > Which is: 5 > > ARM has a much weaker memory model than x86 (search for "arm memory model", e.g. > http://events.linuxfoundation.org/sites/events/files/slides/weak-to-weedy.pdf). > The previous code used a lock, which guarantees all kinds of things – working > with atomics is subtle (hence the name). The Barrier version probably does the > trick, but that's why we prefer locks over atomics. Thanks for the link. This is indeed more subtle than I realized.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1167863002/#ps140001 (title: "Use a barrier FAA.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167863002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) thakis: Nope, still fails. This CL changes the unit test to actually test the sharing part of shared memory. Is it possible that that doesn't actually work on Android? (I'm not familiar with the OS. Let me try asking some Android people).
On 2015/06/12 00:19:20, erikchen wrote: > On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > thakis: Nope, still fails. This CL changes the unit test to actually test the > sharing part of shared memory. Is it possible that that doesn't actually work on > Android? (I'm not familiar with the OS. Let me try asking some Android people). (Note that the test waits for the child processes to finish running, so even if there are race conditions, we'd expect the output to be somewhere between 1 and 5, not 0. The only reason I can think of for the output being 0 is if named shared memory doesn't work on Android.)
On 2015/06/12 00:24:42, erikchen wrote: > On 2015/06/12 00:19:20, erikchen wrote: > > On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > thakis: Nope, still fails. This CL changes the unit test to actually test the > > sharing part of shared memory. Is it possible that that doesn't actually work > on > > Android? (I'm not familiar with the OS. Let me try asking some Android > people). > > (Note that the test waits for the child processes to finish running, so even if > there are race conditions, we'd expect the output to be somewhere between 1 and > 5, not 0. The only reason I can think of for the output being 0 is if named > shared memory doesn't work on Android.) *Given that we're using the barrier version of FAA, that is.
On 2015/06/12 00:25:11, erikchen wrote: > On 2015/06/12 00:24:42, erikchen wrote: > > On 2015/06/12 00:19:20, erikchen wrote: > > > On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > thakis: Nope, still fails. This CL changes the unit test to actually test > the > > > sharing part of shared memory. Is it possible that that doesn't actually > work > > on > > > Android? (I'm not familiar with the OS. Let me try asking some Android > > people). > > > > (Note that the test waits for the child processes to finish running, so even > if > > there are race conditions, we'd expect the output to be somewhere between 1 > and > > 5, not 0. The only reason I can think of for the output being 0 is if named > > shared memory doesn't work on Android.) > > *Given that we're using the barrier version of FAA, that is. Quoting reveman: """ I haven't looked at the test in detail but ashmem used for shared memory on Android does not support named shared memory afaik. I think ashmem segments can only be shared using sendmsg/recvmsg or forking and dup-ing the fd. """ I'll probably just disable the test on Android (since it used to not test anything relevant, anyways).
On 2015/06/18 02:24:07, erikchen wrote: > On 2015/06/12 00:25:11, erikchen wrote: > > On 2015/06/12 00:24:42, erikchen wrote: > > > On 2015/06/12 00:19:20, erikchen wrote: > > > > On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > thakis: Nope, still fails. This CL changes the unit test to actually test > > the > > > > sharing part of shared memory. Is it possible that that doesn't actually > > work > > > on > > > > Android? (I'm not familiar with the OS. Let me try asking some Android > > > people). > > > > > > (Note that the test waits for the child processes to finish running, so even > > if > > > there are race conditions, we'd expect the output to be somewhere between 1 > > and > > > 5, not 0. The only reason I can think of for the output being 0 is if named > > > shared memory doesn't work on Android.) > > > > *Given that we're using the barrier version of FAA, that is. > > Quoting reveman: > """ > I haven't looked at the test in detail but ashmem used for shared memory on > Android does not support named shared memory afaik. I think ashmem segments can > only be shared using sendmsg/recvmsg or forking and dup-ing the fd. > """ > > I'll probably just disable the test on Android (since it used to not test > anything relevant, anyways). Sounds good. If you think the test isn't that useful, I'm fine with deleting it altogether too. Thanks for following up!
On 2015/06/18 02:29:38, Nico wrote: > On 2015/06/18 02:24:07, erikchen wrote: > > On 2015/06/12 00:25:11, erikchen wrote: > > > On 2015/06/12 00:24:42, erikchen wrote: > > > > On 2015/06/12 00:19:20, erikchen wrote: > > > > > On 2015/06/12 00:16:37, commit-bot: I haz the power wrote: > > > > > > Try jobs failed on following builders: > > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > > > thakis: Nope, still fails. This CL changes the unit test to actually > test > > > the > > > > > sharing part of shared memory. Is it possible that that doesn't actually > > > work > > > > on > > > > > Android? (I'm not familiar with the OS. Let me try asking some Android > > > > people). > > > > > > > > (Note that the test waits for the child processes to finish running, so > even > > > if > > > > there are race conditions, we'd expect the output to be somewhere between > 1 > > > and > > > > 5, not 0. The only reason I can think of for the output being 0 is if > named > > > > shared memory doesn't work on Android.) > > > > > > *Given that we're using the barrier version of FAA, that is. > > > > Quoting reveman: > > """ > > I haven't looked at the test in detail but ashmem used for shared memory on > > Android does not support named shared memory afaik. I think ashmem segments > can > > only be shared using sendmsg/recvmsg or forking and dup-ing the fd. > > """ > > > > I'll probably just disable the test on Android (since it used to not test > > anything relevant, anyways). > > Sounds good. If you think the test isn't that useful, I'm fine with deleting it > altogether too. > > Thanks for following up! Of all of the unit tests for shared memory, I think this one is the most useful, because it actually tests sharing memory between multiple processes.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1167863002/#ps180001 (title: "Disable test on Android.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167863002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1167863002/#ps200001 (title: "Remove constant from android translation unit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167863002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/893dadc6e82a64c8135b6f4852ee0960ea62e4a5 Cr-Commit-Position: refs/heads/master@{#335135} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
