Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555483002:
Add POSIX shared memory support for Mac (Closed)
|
Created:
4 years ago by lawrencewu Modified:
3 years, 7 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, vmpstr+watch_chromium.org, viettrungluu+watch_chromium.org, jam, gavinp+memory_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, mac-reviews_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd POSIX shared memory support for Mac
This re-adds POSIX shared memory support on Mac. This
is for use in the sharing of field trials using shared memory. You
can see the design doc for this project at:
https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/
and the motivation for this change under "Port to other platforms".
NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO.
BUG=671228
NOPRESUBMIT=true
Committed: https://crrev.com/2fc6cf8d316d1f452dd27dbd0d886aef15b69659
Cr-Commit-Position: refs/heads/master@{#437709}
Patch Set 1 #Patch Set 2 : Remove unsupported check. #Patch Set 3 : Fix SharedMemoryCreateOptions and make default mach. #Patch Set 4 : Use std::unique_ptr instead of scoped_ptr #Patch Set 5 : Fix compile error on NaCl. #Patch Set 6 : Check that we never pass POSIX shm over Mojo. #
Total comments: 8
Patch Set 7 : Remove unnecessary functionality. #Patch Set 8 : Remove ifdef change in nacl_ipc_adapter.cc #Patch Set 9 : Fully remove IPC POSIX checks. #
Total comments: 6
Patch Set 10 : Remove GetType(). #
Total comments: 13
Patch Set 11 : Remove some more public functions. #Patch Set 12 : Create shared_memory_helper.h and move functions. #Patch Set 13 : Fix posix includes. #Patch Set 14 : Fix some compile issues. #Patch Set 15 : Remove ScopedTracker and fix includes. #Patch Set 16 : Fix ifdef. #Patch Set 17 : Fix compile issues and remove more ScopedTrackers #Patch Set 18 : Readd Android ifdef. #
Total comments: 8
Patch Set 19 : Fix base_unittests. #Patch Set 20 : Ignore shared_memory_helper on Windows. #
Total comments: 22
Patch Set 21 : erikchen@ comments. #
Total comments: 2
Patch Set 22 : Address erikchen@ nits #
Total comments: 11
Patch Set 23 : Address @asvitkine nits #
Total comments: 29
Patch Set 24 : Address mark@ comments. #Patch Set 25 : Restructure CreateAnonymousSharedMemory #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 140 (97 generated)
The CQ bit was checked by lawrencewu@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 lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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...
Description was changed from ========== Revert "Remove POSIX shared memory." This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. BUG=671228 ========== to ========== Revert "Remove POSIX shared memory." This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org, erikchen@chromium.org, rsesek@chromium.org
Description was changed from ========== Revert "Remove POSIX shared memory." This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Revert "Remove POSIX shared memory." This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/12/06 18:15:48, lawrencewu wrote: I'm sorry, but I don't think we should land your CL as-is. 1) Your design needs to go through design review. I don't see why we should land any code before we've agreed on the high-level approach. 2) Given that this is your last week, I think there is a non-trivial probability that the refactor will not land. This will leave base/ in an inappropriate state.
Overall comments: Please only add the functionality that you need. - You don't need IPC - I don't think you need GetType(), since you can write the assumption about the underlying type into the surrounding code [the histogram class, or something]. This means you should't need to change NaCl, IPC code, etc. This is my guess based on the current design, though this may change if the design changes [e.g. if we decide we want to pass more things over early-initialization startup]. - Please refactor functionality shared with POSIX implementation so that there isn't a large amount of copy-pasted code. - Please make sure that POSIX SHM tests run appropriately on Mac. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:134: bool CreateAndMapAnonymousPosix(size_t size); Do you need both these methods? Please only add back the minimum functionality needed to support your desired use case, both here and in the rest of the CL. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:96: static const int TypeMax = 2; Is TypeMax needed? Since you don't need IPC, I think you don't need this, or TypeWireFormat, etc. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:112: SharedMemoryHandle(int fd, bool auto_close); I think you probably don't need line #112, right? https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:140: Type GetType() const; Given that there's only 1 producer/consumer of POSIX SHM, do we really need this as a public method?
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I removed the IPC code, and made GetType() private, since while we don't need GetType() in other places, code in shared_memory_mac.cc still uses it (so I don't think we can remove GetType() completely). The shared memory unittests should run using POSIX, and the Mach tests should run in shared_memory_mac_unittest.cc. As for the shared functionality, while I agree there is some copy-pasted code, I'm not totally how we should refactor this. Maybe write an empty helper class that just contains some stateless functions like CreateAnonymousSharedMemory()? https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:134: bool CreateAndMapAnonymousPosix(size_t size); On 2016/12/06 19:00:41, erikchen wrote: > Do you need both these methods? > > Please only add back the minimum functionality needed to support your desired > use case, both here and in the rest of the CL. Removed. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:96: static const int TypeMax = 2; On 2016/12/06 19:00:41, erikchen wrote: > Is TypeMax needed? Since you don't need IPC, I think you don't need this, or > TypeWireFormat, etc. Removed. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:112: SharedMemoryHandle(int fd, bool auto_close); On 2016/12/06 19:00:41, erikchen wrote: > I think you probably don't need line #112, right? Nope, removed and made existing calls use the first ctor. https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_handle.h:140: Type GetType() const; On 2016/12/06 19:00:41, erikchen wrote: > Given that there's only 1 producer/consumer of POSIX SHM, do we really need this > as a public method? Nope, made private.
On 2016/12/06 21:04:41, lawrencewu wrote: > I removed the IPC code, and made GetType() private, since while we don't need > GetType() in other places, code in shared_memory_mac.cc still uses it (so I > don't > think we can remove GetType() completely). The shared memory unittests should > run using POSIX, and the Mach tests should run in shared_memory_mac_unittest.cc. > > As for the shared functionality, while I agree there is some copy-pasted code, > I'm not totally how we should refactor this. Maybe write an empty helper class > that > just contains some stateless functions like CreateAnonymousSharedMemory()? > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > File base/memory/shared_memory.h (right): > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > base/memory/shared_memory.h:134: bool CreateAndMapAnonymousPosix(size_t size); > On 2016/12/06 19:00:41, erikchen wrote: > > Do you need both these methods? > > > > Please only add back the minimum functionality needed to support your desired > > use case, both here and in the rest of the CL. > > Removed. > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > File base/memory/shared_memory_handle.h (right): > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_handle.h:96: static const int TypeMax = 2; > On 2016/12/06 19:00:41, erikchen wrote: > > Is TypeMax needed? Since you don't need IPC, I think you don't need this, or > > TypeWireFormat, etc. > > Removed. > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_handle.h:112: SharedMemoryHandle(int fd, bool > auto_close); > On 2016/12/06 19:00:41, erikchen wrote: > > I think you probably don't need line #112, right? > > Nope, removed and made existing calls use the first ctor. > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_handle.h:140: Type GetType() const; > On 2016/12/06 19:00:41, erikchen wrote: > > Given that there's only 1 producer/consumer of POSIX SHM, do we really need > this > > as a public method? > > Nope, made private. Actually, I just removed GetType() and made SharedMemory check for type_.
The CQ bit was checked by lawrencewu@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...
On 2016/12/06 21:10:19, lawrencewu wrote: > On 2016/12/06 21:04:41, lawrencewu wrote: > > I removed the IPC code, and made GetType() private, since while we don't need > > GetType() in other places, code in shared_memory_mac.cc still uses it (so I > > don't > > think we can remove GetType() completely). The shared memory unittests should > > run using POSIX, and the Mach tests should run in > shared_memory_mac_unittest.cc. > > > > As for the shared functionality, while I agree there is some copy-pasted code, > > I'm not totally how we should refactor this. Maybe write an empty helper class > > that > > just contains some stateless functions like CreateAnonymousSharedMemory()? Right. I suspect most shared functionality can be made stateless. > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > File base/memory/shared_memory.h (right): > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory.h:134: bool CreateAndMapAnonymousPosix(size_t size); > > On 2016/12/06 19:00:41, erikchen wrote: > > > Do you need both these methods? > > > > > > Please only add back the minimum functionality needed to support your > desired > > > use case, both here and in the rest of the CL. > > > > Removed. > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > File base/memory/shared_memory_handle.h (right): > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_handle.h:96: static const int TypeMax = 2; > > On 2016/12/06 19:00:41, erikchen wrote: > > > Is TypeMax needed? Since you don't need IPC, I think you don't need this, or > > > TypeWireFormat, etc. > > > > Removed. > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_handle.h:112: SharedMemoryHandle(int fd, bool > > auto_close); > > On 2016/12/06 19:00:41, erikchen wrote: > > > I think you probably don't need line #112, right? > > > > Nope, removed and made existing calls use the first ctor. > > > > > https://codereview.chromium.org/2555483002/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_handle.h:140: Type GetType() const; > > On 2016/12/06 19:00:41, erikchen wrote: > > > Given that there's only 1 producer/consumer of POSIX SHM, do we really need > > this > > > as a public method? > > > > Nope, made private. > > Actually, I just removed GetType() and made SharedMemory check for type_.
much better. https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:140: void SetFileHandle(int fd, bool auto_close); Is this method necessary? auto_close is a parameter only used by IPC, IIRC. https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle.h:145: const FileDescriptor GetFileDescriptor() const; Do we need both this and GetFDFromSharedMemoryHandle? Could we remove the latter? https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:73: return SharedMemoryHandle(); do we need duplicate?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_handle.h:88: // The values of these enums must not change, as they are used by the Is this comment still relevant? I don't think we need to histogram anymore. https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:87: DCHECK_EQ(kr, KERN_SUCCESS); Can't this create an invalid handle, since this is only a DCHECK? https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:87: DCHECK_EQ(kr, KERN_SUCCESS); Should there be logging here, similar to ::Close() ? https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:203: DPLOG(ERROR) << "Error deallocating mach port: " << kr; DPLOG isn't appropriate here. MACH_LOG is. https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:87: tracked_objects::ScopedTracker tracking_profile( Do we still need the ScopedTrackers in this file? https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:108: // A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU Hmmmm. I wonder if we could use shm_open for this instead, since the 4mb limit shouldn't be an issue. It may be faster than the unlink-tmp-file approach.
The CQ bit was checked by lawrencewu@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...
Address comments and create shared_memory_helper.{cc, h} in base/memory/. I
still have to move SharedMemory::ShareToProcessCommon() there and fix those
failing tests.
https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):
https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:140: void SetFileHandle(int fd, bool
auto_close);
On 2016/12/06 21:14:50, erikchen wrote:
> Is this method necessary? auto_close is a parameter only used by IPC, IIRC.
Removed but had to modify the call from SharedMemory::ShareToProcessCommon() by
replacing it with its body.
https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:145: const FileDescriptor GetFileDescriptor()
const;
On 2016/12/06 21:14:50, erikchen wrote:
> Do we need both this and GetFDFromSharedMemoryHandle? Could we remove the
> latter?
I assume by latter you mean GetFileDescriptor()? Removed and replaced the calls
in SharedMemory.
https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem...
File base/memory/shared_memory_handle_mac.cc (right):
https://codereview.chromium.org/2555483002/diff/160001/base/memory/shared_mem...
base/memory/shared_memory_handle_mac.cc:73: return SharedMemoryHandle();
On 2016/12/06 21:14:50, erikchen wrote:
> do we need duplicate?
No. Removed the switch statement and added a comment in the header saying this
method assumes the SharedMemoryHandle is of type Mach.
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
File base/memory/shared_memory_handle.h (right):
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_handle.h:88: // The values of these enums must not
change, as they are used by the
On 2016/12/06 21:45:55, Robert Sesek wrote:
> Is this comment still relevant? I don't think we need to histogram anymore.
Don't think so -- removed.
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
File base/memory/shared_memory_handle_mac.cc (right):
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_handle_mac.cc:87: DCHECK_EQ(kr, KERN_SUCCESS);
On 2016/12/06 21:45:55, Robert Sesek wrote:
> Should there be logging here, similar to ::Close() ?
Added logging.
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_handle_mac.cc:87: DCHECK_EQ(kr, KERN_SUCCESS);
On 2016/12/06 21:45:55, Robert Sesek wrote:
> Can't this create an invalid handle, since this is only a DCHECK?
Yes, I suppose so. I added a CHECK(false) here.
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_handle_mac.cc:203: DPLOG(ERROR) << "Error deallocating
mach port: " << kr;
On 2016/12/06 21:45:55, Robert Sesek wrote:
> DPLOG isn't appropriate here. MACH_LOG is.
Fixed.
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (right):
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:87: tracked_objects::ScopedTracker
tracking_profile(
On 2016/12/06 21:45:55, Robert Sesek wrote:
> Do we still need the ScopedTrackers in this file?
Probably not, now that the bug is fixed. erikchen@, can you confirm?
https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:108: // A: Because they're limited to 4mb on OS
X. FFFFFFFUUUUUUUUUUU
On 2016/12/06 21:45:55, Robert Sesek wrote:
> Hmmmm. I wonder if we could use shm_open for this instead, since the 4mb limit
> shouldn't be an issue. It may be faster than the unlink-tmp-file approach.
I'd like to punt on this, since we can just reuse the POSIX code here and I
don't think I have time
to re-implement this using shm_open.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by lawrencewu@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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
> https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... > base/memory/shared_memory_mac.cc:87: tracked_objects::ScopedTracker > tracking_profile( > On 2016/12/06 21:45:55, Robert Sesek wrote: > > Do we still need the ScopedTrackers in this file? > > Probably not, now that the bug is fixed. erikchen@, can you confirm? Correct, no need for ScopedTrackers. > > https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... > base/memory/shared_memory_mac.cc:108: // A: Because they're limited to 4mb on OS > X. FFFFFFFUUUUUUUUUUU > On 2016/12/06 21:45:55, Robert Sesek wrote: > > Hmmmm. I wonder if we could use shm_open for this instead, since the 4mb limit > > shouldn't be an issue. It may be faster than the unlink-tmp-file approach. > > I'd like to punt on this, since we can just reuse the POSIX code here and I > don't think I have time > to re-implement this using shm_open. I think that moving to shm_open is quite appropriate, since we expect it to have better performance on mac, and it should simplify this code. That being said, the 4MB limit might be relevant - it depends on exactly what happens with the design review. I don't think we should punt on this just because you are time limited. That being said, if you really want to land code by end of week, you should be coordinating with base OWNERs and get them to approve your code. I recommend mark@ and thakis@, as BASE owners most familiar with Mac.
The CQ bit was checked by lawrencewu@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 lawrencewu@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 lawrencewu@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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...
https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:70: SharedMemoryHandle SharedMemoryHandle::Duplicate() const { Does this need an implementation for type_==POSIX ? https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:79: CHECK(false); There's MACH_CHECK, but this could maybe do the return like at line 72. https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_helper.h:24: typedef ScopedGeneric<FilePath*, ScopedPathUnlinkerTraits> ScopedPathUnlinker; typedef -> using statement https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:315: DPLOG(ERROR) << "dup() failed."; The DPLOG should go ahead of the if because errno may change as a result of those functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lawrencewu@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...
On 2016/12/06 23:08:32, erikchen wrote: > > > https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... > > base/memory/shared_memory_mac.cc:87: tracked_objects::ScopedTracker > > tracking_profile( > > On 2016/12/06 21:45:55, Robert Sesek wrote: > > > Do we still need the ScopedTrackers in this file? > > > > Probably not, now that the bug is fixed. erikchen@, can you confirm? > Correct, no need for ScopedTrackers. > > > > > > https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... > > base/memory/shared_memory_mac.cc:108: // A: Because they're limited to 4mb on > OS > > X. FFFFFFFUUUUUUUUUUU > > On 2016/12/06 21:45:55, Robert Sesek wrote: > > > Hmmmm. I wonder if we could use shm_open for this instead, since the 4mb > limit > > > shouldn't be an issue. It may be faster than the unlink-tmp-file approach. > > > > I'd like to punt on this, since we can just reuse the POSIX code here and I > > don't think I have time > > to re-implement this using shm_open. > > I think that moving to shm_open is quite appropriate, since we expect it to have > better performance on mac, and it should simplify this code. That being said, > the 4MB limit might be relevant - it depends on exactly what happens with the > design review. I don't think we should punt on this just because you are time > limited. > > That being said, if you really want to land code by end of week, you should be > coordinating with base OWNERs and get them to approve your code. I recommend > mark@ and thakis@, as BASE owners most familiar with Mac. Ack, will add mark@ and thakis@ to reviewers.
lawrencewu@chromium.org changed reviewers: + mark@chromium.org, thakis@chromium.org
Address comments and fix failing unittests. mark@, thakis@: I'm trying to land this CL before the end of this week (which is when my internship ends). While ideally we'd use shm_open now for shared memory on Mac, I'm not sure if I have time to implement it. Wondering if you'd approve this code so I can finish up the last part of my project (which is enabling using shared memory for field trial state). https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:87: tracked_objects::ScopedTracker tracking_profile( On 2016/12/06 22:33:24, lawrencewu wrote: > On 2016/12/06 21:45:55, Robert Sesek wrote: > > Do we still need the ScopedTrackers in this file? > > Probably not, now that the bug is fixed. erikchen@, can you confirm? Removed all instances of ScopedTracker. https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:70: SharedMemoryHandle SharedMemoryHandle::Duplicate() const { On 2016/12/07 16:39:42, Robert Sesek wrote: > Does this need an implementation for type_==POSIX ? No, since my project doesn't use this. There's a comment in the header file saying that it assumes the handle is of type Mach. https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:79: CHECK(false); On 2016/12/07 16:39:42, Robert Sesek wrote: > There's MACH_CHECK, but this could maybe do the return like at line 72. Done. https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_helper.h:24: typedef ScopedGeneric<FilePath*, ScopedPathUnlinkerTraits> ScopedPathUnlinker; On 2016/12/07 16:39:42, Robert Sesek wrote: > typedef -> using statement Changed, but doesn't the style guide say to avoid using 'using' statements? https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/340001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:315: DPLOG(ERROR) << "dup() failed."; On 2016/12/07 16:39:42, Robert Sesek wrote: > The DPLOG should go ahead of the if because errno may change as a result of > those functions. Done.
We just got rid of this. What, exactly, do you need to resurrect it for? Why does the shared memory implementation we have as it exists now not work for you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/12/07 19:56:05, Mark Mentovai wrote: > We just got rid of this. What, exactly, do you need to resurrect it for? Why > does the shared memory implementation we have as it exists now not work for you? We've looked into using the Mach APIs. Since it would require adding another synchronous call at startup, and using fd-backed shared memory is easier, I think we've decided to go with this approach. You can check out the design doc for more info, and I'll also cc you on the email thread discussing this.
On 2016/12/07 20:04:44, lawrencewu wrote: > On 2016/12/07 19:56:05, Mark Mentovai wrote: > > We just got rid of this. What, exactly, do you need to resurrect it for? Why > > does the shared memory implementation we have as it exists now not work for > you? > > We've looked into using the Mach APIs. Since it would require adding another > synchronous call at startup, and using fd-backed shared memory is easier, > I think we've decided to go with this approach. You can check out the design > doc for more info, and I'll also cc you on the email thread discussing this. friendly ping
Is the dependent code in good shape that’s ready to land?
On 2016/12/08 20:58:07, Mark Mentovai wrote: > Is the dependent code in good shape that’s ready to land? Yes -- it's very short, since we do nearly the same thing on Linux already. https://codereview.chromium.org/2555453002/
OK. Erik, do you want to take another look?
The CQ bit was checked by lawrencewu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Please update your CL title/description. As noted earlier, this CL review is commencing in parallel to design review. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory.h:260: #if !(defined(OS_MACOSX) && !defined(OS_IOS)) not sure why the preprocessor conditional needs to be separated in two? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:17: #include <sys/types.h> why is this necessary? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:57: CopyRelevantData(handle); can we modify CopyRelevantData to also copy the type? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: if (kr != KERN_SUCCESS) { Why is this not a DCHECK? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:96: return file_descriptor_ == handle.file_descriptor_; this compares the "auto_close" parameter, which we don't want. We just want to compare the fd. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:21: // A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU as indicated in a previous round of comments, it seems like we can use shm_open on Mac, which should simplify a lot of this logic? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:194: &readonly_mapped_file_); Do we still need readonly_mapped_file_? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:307: handle_to_dup = readonly_mapped_file_; Do we still need both "readonly_mapped_file_" and "shm_.file_descriptor_"? Seems redundant for your use case? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:314: if (close_self) { This close_self logic is duplicated in three places in this function. Please refactor.
I don’t think that it’s the worst thing to hold this until the design review is complete.
I don't think there are any pending concerns on the design review. Justin looked at it yesterday and the main comment he had is it would be good to generalize this for other features to use, which I think is a good follow-up step and we'd like to do that for persistent histograms in the future anyway - so it can be generalized then (i.e. moving some parts that we have in the field trial code to somewhere more generic in base). On Thu, Dec 8, 2016 at 4:35 PM, <mark@chromium.org> wrote: > I don’t think that it’s the worst thing to hold this until the design > review is > complete. > > https://codereview.chromium.org/2555483002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by lawrencewu@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...
Address comments. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory.h:260: #if !(defined(OS_MACOSX) && !defined(OS_IOS)) On 2016/12/08 21:33:33, erikchen wrote: > not sure why the preprocessor conditional needs to be separated in two? it doesn't -- reverted. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle.h:17: #include <sys/types.h> On 2016/12/08 21:33:33, erikchen wrote: > why is this necessary? it's not -- removed. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:57: CopyRelevantData(handle); On 2016/12/08 21:33:33, erikchen wrote: > can we modify CopyRelevantData to also copy the type? Done. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: if (kr != KERN_SUCCESS) { On 2016/12/08 21:33:34, erikchen wrote: > Why is this not a DCHECK? Added a DCHECK. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:96: return file_descriptor_ == handle.file_descriptor_; On 2016/12/08 21:33:33, erikchen wrote: > this compares the "auto_close" parameter, which we don't want. We just want to > compare the fd. Done. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:21: // A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU On 2016/12/08 21:33:34, erikchen wrote: > as indicated in a previous round of comments, it seems like we can use shm_open > on Mac, which should simplify a lot of this logic? Won't this also require undeprecating the `name` and `open_existing` fields in SharedMemoryCreateOptions? I'm almost certain I won't have time to implement this. Perhaps you or asvitkine@ can carry this forward? https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:194: &readonly_mapped_file_); On 2016/12/08 21:33:34, erikchen wrote: > Do we still need readonly_mapped_file_? I think so -- we're sharing the fd as readonly: https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?q=field_tria... so actually readonly_mapped_file is the field getting populated. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:307: handle_to_dup = readonly_mapped_file_; On 2016/12/08 21:33:34, erikchen wrote: > Do we still need both "readonly_mapped_file_" and "shm_.file_descriptor_"? Seems > redundant for your use case? I think so -- the browser process uses readonly_mapped_file_, while the child process will still use shm_.file_descriptor_. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:314: if (close_self) { On 2016/12/08 21:33:34, erikchen wrote: > This close_self logic is duplicated in three places in this function. Please > refactor. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Okay. This CL looks fine. The shm_open changes do not have to be a part of this CL [and honestly should be made separately anyways, since they might have fallout]. lgtm % nits. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:21: // A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU On 2016/12/08 23:06:30, lawrencewu wrote: > On 2016/12/08 21:33:34, erikchen wrote: > > as indicated in a previous round of comments, it seems like we can use > shm_open > > on Mac, which should simplify a lot of this logic? > > Won't this also require undeprecating the `name` and `open_existing` fields in > SharedMemoryCreateOptions? I'm almost certain I won't have time to implement > this. Perhaps you or asvitkine@ can carry this forward? Note that the current mechanism makes an actual file on disk [with a random name], and then unlinks it. You could do the same thing with shm_open and shm_unlink and everything *should work*. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:314: if (close_self) { On 2016/12/08 23:06:30, lawrencewu wrote: > On 2016/12/08 21:33:34, erikchen wrote: > > This close_self logic is duplicated in three places in this function. Please > > refactor. > > Done. Perhaps you could pull all the logic for handle duplication into a separate method, and then in this method. bool success = Duplicate handle(). if (close_self) { Unmap(); Close(); } return success; This avoids a common type of error where we return from the function but forget to call UnmapAndCloseIfNeeded. https://codereview.chromium.org/2555483002/diff/400001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/400001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: DCHECK_EQ(kr, KERN_SUCCESS); this line doesn't make any sense, since it will always trigger in debug builds. My question is why you changed this from a DCHECK into a conditional. If we can't call mach_port_mod_refs on a memory object, something is really broken, and there is really no way we can safely *recover*.
Oh, please update your CL description/title. This CL is not just a revert - it has fairly substantial changes.
Description was changed from ========== Revert "Remove POSIX shared memory." This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
Description was changed from ========== Add POSIX shared memory support for Mac This reverts commit 21fef176d164d23237563e76a280f5012fd5c425. This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
On 2016/12/09 00:31:13, erikchen wrote: > Oh, please update your CL description/title. This CL is not just a revert - it > has fairly substantial changes. Done.
The CQ bit was checked by lawrencewu@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...
Address nits. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:21: // A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU On 2016/12/09 00:30:38, erikchen wrote: > On 2016/12/08 23:06:30, lawrencewu wrote: > > On 2016/12/08 21:33:34, erikchen wrote: > > > as indicated in a previous round of comments, it seems like we can use > > shm_open > > > on Mac, which should simplify a lot of this logic? > > > > Won't this also require undeprecating the `name` and `open_existing` fields in > > SharedMemoryCreateOptions? I'm almost certain I won't have time to implement > > this. Perhaps you or asvitkine@ can carry this forward? > > Note that the current mechanism makes an actual file on disk [with a random > name], and then unlinks it. You could do the same thing with shm_open and > shm_unlink and everything *should work*. Acknowledged. https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... File base/memory/shared_memory_mac.cc (right): https://codereview.chromium.org/2555483002/diff/380001/base/memory/shared_mem... base/memory/shared_memory_mac.cc:314: if (close_self) { On 2016/12/09 00:30:38, erikchen wrote: > On 2016/12/08 23:06:30, lawrencewu wrote: > > On 2016/12/08 21:33:34, erikchen wrote: > > > This close_self logic is duplicated in three places in this function. Please > > > refactor. > > > > Done. > > Perhaps you could pull all the logic for handle duplication into a separate > method, and then in this method. > > bool success = Duplicate handle(). > if (close_self) { > Unmap(); > Close(); > } > return success; > > This avoids a common type of error where we return from the function but forget > to call UnmapAndCloseIfNeeded. Yeah, that is cleaner. Done. https://codereview.chromium.org/2555483002/diff/400001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/400001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: DCHECK_EQ(kr, KERN_SUCCESS); On 2016/12/09 00:30:38, erikchen wrote: > this line doesn't make any sense, since it will always trigger in debug builds. > My question is why you changed this from a DCHECK into a conditional. If we > can't call mach_port_mod_refs on a memory object, something is really broken, > and there is really no way we can safely *recover*. Okay, I changed it back to a DCHECK. The initial motivation to change it to a conditional was rsesek's comment: https://codereview.chromium.org/2555483002/diff/180001/base/memory/shared_mem... to add logging, similar to the ::Close().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % comments Please add a blank line in your CL description after the first line. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: Nit: Remove extra diff. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: No (c) https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:35: if (options.share_read_only) { Nit: Combine with the above if. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:78: } else { Nit: No else since you return above. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: No (c) https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.h:37: bool PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd, int* mapped_file, Add a comment.
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Address nits. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:77: On 2016/12/09 18:04:15, Alexei Svitkine (slow) wrote: > Nit: Remove extra diff. Done. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/12/09 18:04:15, Alexei Svitkine (slow) wrote: > Nit: No (c) Done. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:35: if (options.share_read_only) { On 2016/12/09 18:04:15, Alexei Svitkine (slow) wrote: > Nit: Combine with the above if. Done. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:78: } else { On 2016/12/09 18:04:15, Alexei Svitkine (slow) wrote: > Nit: No else since you return above. Done. https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/420001/base/memory/shared_mem... base/memory/shared_memory_helper.h:37: bool PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd, int* mapped_file, On 2016/12/09 18:04:16, Alexei Svitkine (slow) wrote: > Add a comment. Done.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lawrencewu@chromium.org changed reviewers: - thakis@chromium.org
mark@: Mind taking a look?
https://codereview.chromium.org/2555483002/diff/440001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2555483002/diff/440001/base/BUILD.gn#newcode1 base/BUILD.gn:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved. CL description: > This re-adds POSIX shared memory support on MAC. This Mac (or macOS) > is for use in the sharing of field trials using shared memory. You > can see the design doc for this project at go/share-field-trials, and This is an internal link. Is the doc available publicly? Please use a link that works outside of Google. Is the doc not available publicly? Consider making it available publicly, or removing the link altogether and perhaps putting the relevant content from the doc in-line here. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle.h:125: // of type Mach. MACH https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:147: return *memory && *memory != reinterpret_cast<void*>(-1); This should just be return *memory != MAP_FAILED; The check for null is invalid for mmap(), and you don’t need the funny cast because the MAP_FAILED constant already exists for this purpose. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:173: DPLOG(ERROR) << "Error closing fd."; nit: no period https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:178: MACH_LOG(ERROR, kr) << "Error deallocating mach port"; This was a DPLOG before. The P was incorrect, but I don’t know if the D was. Notably, you’re using DPLOG for the POSIX case above (correctly with P in that case). So I think that this one should be MACH_DLOG for consistency. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:31: path_unlinker.reset(path); So you unlink path here… https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:36: readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY))); …then will you try to reopen it here? https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:59: if (fstat(fileno(fp.get()), &st)) What’s the point of doing this if readonly_fd.is_valid() isn’t true? https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:73: if (errno == EMFILE) { Is it worth distinguishing EMFILE? dup() failing is bad no matter what. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:14: struct ScopedPathUnlinkerTraits { Does this need to be in the header? Doesn’t look like it. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:15: static FilePath* InvalidValue() { return nullptr; } Can probably use const FilePath for this unlinker stuff https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:24: using ScopedPathUnlinker = ScopedGeneric<FilePath*, ScopedPathUnlinkerTraits>; (through here)
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at go/share-field-trials, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/, and the motivation for this change under "Port to other platforms". BUG=671228 ==========
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/, and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ , and the motivation for this change under "Port to other platforms". BUG=671228 ==========
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ , and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". BUG=671228 ==========
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on MAC. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". BUG=671228 ==========
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Address mark@ comments. https://codereview.chromium.org/2555483002/diff/440001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2555483002/diff/440001/base/BUILD.gn#newcode1 base/BUILD.gn:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/12/09 19:53:52, Mark Mentovai wrote: > CL description: > > > This re-adds POSIX shared memory support on MAC. This > > Mac (or macOS) > > > is for use in the sharing of field trials using shared memory. You > > can see the design doc for this project at go/share-field-trials, and > > This is an internal link. Is the doc available publicly? Please use a link that > works outside of Google. Is the doc not available publicly? Consider making it > available publicly, or removing the link altogether and perhaps putting the > relevant content from the doc in-line here. Changed MAC to Mac. Also made the doc publicly available on my chromium account and changed the link to point to that. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle.h:125: // of type Mach. On 2016/12/09 19:53:52, Mark Mentovai wrote: > MACH Done. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_handle_mac.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:147: return *memory && *memory != reinterpret_cast<void*>(-1); On 2016/12/09 19:53:52, Mark Mentovai wrote: > This should just be > > return *memory != MAP_FAILED; > > The check for null is invalid for mmap(), and you don’t need the funny cast > because the MAP_FAILED constant already exists for this purpose. Done. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:173: DPLOG(ERROR) << "Error closing fd."; On 2016/12/09 19:53:52, Mark Mentovai wrote: > nit: no period Done. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_handle_mac.cc:178: MACH_LOG(ERROR, kr) << "Error deallocating mach port"; On 2016/12/09 19:53:52, Mark Mentovai wrote: > This was a DPLOG before. The P was incorrect, but I don’t know if the D was. > Notably, you’re using DPLOG for the POSIX case above (correctly with P in that > case). So I think that this one should be MACH_DLOG for consistency. Done. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:36: readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY))); On 2016/12/09 19:53:52, Mark Mentovai wrote: > …then will you try to reopen it here? I think we have to re-open it as readonly here. Are you suggesting having two separate codepaths so one doesn't have to re-open? https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:59: if (fstat(fileno(fp.get()), &st)) On 2016/12/09 19:53:52, Mark Mentovai wrote: > What’s the point of doing this if readonly_fd.is_valid() isn’t true? Moved to inside the if statement. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:73: if (errno == EMFILE) { On 2016/12/09 19:53:52, Mark Mentovai wrote: > Is it worth distinguishing EMFILE? dup() failing is bad no matter what. Removed the check for EMFILE. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.h (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:14: struct ScopedPathUnlinkerTraits { On 2016/12/09 19:53:53, Mark Mentovai wrote: > Does this need to be in the header? Doesn’t look like it. Moved to the .cc file. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:15: static FilePath* InvalidValue() { return nullptr; } On 2016/12/09 19:53:53, Mark Mentovai wrote: > Can probably use const FilePath for this unlinker stuff Done. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.h:24: using ScopedPathUnlinker = ScopedGeneric<FilePath*, ScopedPathUnlinkerTraits>; On 2016/12/09 19:53:53, Mark Mentovai wrote: > (through here) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:36: readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY))); lawrencewu wrote: > On 2016/12/09 19:53:52, Mark Mentovai wrote: > > …then will you try to reopen it here? > > I think we have to re-open it as readonly here. Are you suggesting having two > separate codepaths so one doesn't have to re-open? I mean that it may have been unlinked by this point.
https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:36: readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY))); On 2016/12/09 20:54:13, Mark Mentovai wrote: > lawrencewu wrote: > > On 2016/12/09 19:53:52, Mark Mentovai wrote: > > > …then will you try to reopen it here? > > > > I think we have to re-open it as readonly here. Are you suggesting having two > > separate codepaths so one doesn't have to re-open? > > I mean that it may have been unlinked by this point. So the code is kind of confusing but I don't think the path gets unlinked on line 31. The reset will abort before trying to free/unlink the path.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/12/09 21:14:31, lawrencewu wrote: > https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... > File base/memory/shared_memory_helper.cc (right): > > https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... > base/memory/shared_memory_helper.cc:36: > readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY))); > On 2016/12/09 20:54:13, Mark Mentovai wrote: > > lawrencewu wrote: > > > On 2016/12/09 19:53:52, Mark Mentovai wrote: > > > > …then will you try to reopen it here? > > > > > > I think we have to re-open it as readonly here. Are you suggesting having > two > > > separate codepaths so one doesn't have to re-open? > > > > I mean that it may have been unlinked by this point. > > So the code is kind of confusing but I don't think the path gets unlinked on > line 31. The reset will abort before trying to free/unlink the path. Sorry, I don't mean abort, but rather it will try to free/unlink the current value (which is nullptr by default) and _then_ take on the new path's value.
https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:34: if (*fp && options.share_read_only) { OK, I see. A lot of related but distinct objects in here, it’s hard to keep track. Now I’m concerned that *fp might not be false if GetShmemTempDir() failed. Since so much in here is done with out-parameters, they can arrive in this function with any caller-controlled value. I think we should play it safer. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:43: return true; Another problem is that if (!*fp), we probably don’t want to return true. How about restructuring this function to use early returns more instead of so much nested conditional stuff? Aside from options.share_read_only, the only real conditions that are happening in here seem to be testing for failure.
Address mark@ comments. https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:34: if (*fp && options.share_read_only) { On 2016/12/09 21:21:30, Mark Mentovai wrote: > OK, I see. A lot of related but distinct objects in here, it’s hard to keep > track. > > Now I’m concerned that *fp might not be false if GetShmemTempDir() failed. Since > so much in here is done with out-parameters, they can arrive in this function > with any caller-controlled value. I think we should play it safer. OK, I restructured the code so it uses more early returns, and directly check for GetShmemTempDir(). https://codereview.chromium.org/2555483002/diff/440001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:43: return true; On 2016/12/09 21:21:30, Mark Mentovai wrote: > Another problem is that if (!*fp), we probably don’t want to return true. > > How about restructuring this function to use early returns more instead of so > much nested conditional stuff? Aside from options.share_read_only, the only real > conditions that are happening in here seem to be testing for failure. Done.
LGTM
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". BUG=671228 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO. BUG=671228 NOPRESUBMIT=true ==========
The CQ bit was checked by lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2555483002/#ps480001 (title: "Restructure CreateAnonymousSharedMemory")
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": 480001, "attempt_start_ts": 1481321618702370,
"parent_rev": "9020b09f949d57dfcce77248a43353c3247ee6d4", "commit_rev":
"fb2741596ac1bcaec4e91a8b61451ecc25bb911c"}
Message was sent while issue was closed.
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO. BUG=671228 NOPRESUBMIT=true ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO. BUG=671228 NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2555483002 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO. BUG=671228 NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2555483002 ========== to ========== Add POSIX shared memory support for Mac This re-adds POSIX shared memory support on Mac. This is for use in the sharing of field trials using shared memory. You can see the design doc for this project at: https://docs.google.com/document/d/1mTjD99PchmCGYtLvpxaXFObVnid1zmXIsWBI7GjuJgo/ and the motivation for this change under "Port to other platforms". NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO. BUG=671228 NOPRESUBMIT=true Committed: https://crrev.com/2fc6cf8d316d1f452dd27dbd0d886aef15b69659 Cr-Commit-Position: refs/heads/master@{#437709} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/2fc6cf8d316d1f452dd27dbd0d886aef15b69659 Cr-Commit-Position: refs/heads/master@{#437709}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2555483002/diff/480001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/480001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:75: struct stat st = {}; This file is posix-only; why was it not named shared_memory_helper_posix.cc? (I'm currently unbreaking gn bootstrap on Win, and one of the reasons it's broken is because this file isn't in the posix-block in tools/gn/bootstrap/bootstrap.py but in the common block, probably because of its name)
Message was sent while issue was closed.
https://codereview.chromium.org/2555483002/diff/480001/base/memory/shared_mem... File base/memory/shared_memory_helper.cc (right): https://codereview.chromium.org/2555483002/diff/480001/base/memory/shared_mem... base/memory/shared_memory_helper.cc:75: struct stat st = {}; On 2017/05/12 15:11:52, Nico wrote: > This file is posix-only; why was it not named shared_memory_helper_posix.cc? > (I'm currently unbreaking gn bootstrap on Win, and one of the reasons it's > broken is because this file isn't in the posix-block in > tools/gn/bootstrap/bootstrap.py but in the common block, probably because of its > name) Renaming SGTM. (Lawrence was an intern so someone else would have to make this change.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
