Share field trial allocator on Mac

Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/. We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls to AllowIO.

BUG=663912
NOPRESUBMIT=true

Committed: https://crrev.com/d17b445e932a10c875cea034b037b19d30e47a8c
Cr-Commit-Position: refs/heads/master@{#438265}

[lawrencewu, 2016-12-09 22:29:56]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[lawrencewu, 2016-12-09 22:30:17]

Description was changed from

==========
Share field trial allocator on Mac

BUG=663912
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same as
the Linux CL: https://codereview.chromium.org/2530573002/. We just need to
edit some ifdefs.

BUG=663912
==========

[lawrencewu, 2016-12-09 22:31:01]

lawrencewu@chromium.org changed reviewers:
+ asvitkine@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:31:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-12-09 22:35:22]

lgtm

woot!

[lawrencewu, 2016-12-09 22:45:32]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 22:46:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lawrencewu, 2016-12-09 23:32:39]

The CQ bit was unchecked by lawrencewu@chromium.org

[lawrencewu, 2016-12-09 23:33:20]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same as
the Linux CL: https://codereview.chromium.org/2530573002/. We just need to
edit some ifdefs.

BUG=663912
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same as
the Linux CL: https://codereview.chromium.org/2530573002/. We just need to
edit some ifdefs.

NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

[lawrencewu, 2016-12-09 23:33:40]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 23:34:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 01:04:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:04:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lawrencewu, 2016-12-10 01:22:37]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same as
the Linux CL: https://codereview.chromium.org/2530573002/. We just need to
edit some ifdefs.

NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

[lawrencewu, 2016-12-10 02:01:37]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 02:02:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lawrencewu, 2016-12-10 02:05:02]

lawrencewu@chromium.org changed reviewers:
+ mark@chromium.org

[lawrencewu, 2016-12-10 02:05:02]

mark@: I had to readd the POSIX implementation for Duplicate().

I thought I didn't need it for my project, but I forgot the unit tests still use
it (e.g.
https://cs.chromium.org/chromium/src/base/metrics/field_trial_unittest.cc?q=f...).
Please take a look, thanks.

[lawrencewu, 2016-12-10 02:15:29]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because this re-adds some banned calls to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

[lawrencewu, 2016-12-10 02:15:52]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-12-10 03:44:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 03:44:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-12-10 12:48:47]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 12:48:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 12:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 12:56:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lawrencewu, 2016-12-10 15:13:11]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 15:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 17:01:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 17:01:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lawrencewu, 2016-12-10 17:39:42]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 17:39:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 19:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 19:24:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lawrencewu, 2016-12-10 19:26:34]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 19:26:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 20:15:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 20:15:23]

Dry run: This issue passed the CQ dry run.

[lawrencewu, 2016-12-10 23:35:57]

On 2016/12/10 02:05:02, lawrencewu wrote:
> mark@: I had to readd the POSIX implementation for Duplicate().
> 
> I thought I didn't need it for my project, but I forgot the unit tests still
use
> it (e.g.
>
https://cs.chromium.org/chromium/src/base/metrics/field_trial_unittest.cc?q=f...).
> Please take a look, thanks.

asvitkine@: I had to change the method of field trial handle passing on POSIX.

The problem with the current method is some browser tests don't initialize the
allocator, but the child process still tries to open the fd.
Since it can't distinguish anymore whether or not the fd is valid (because it's
a constant), the map will fail. I solved this by passing the
--field-trial-handle flag and making the child check for the presence of the
flag (the value is invalid but it doesn't matter). Let me know
if this is acceptable and I'll update the comments and docs to reflect this.

[Alexei Svitkine (slow), 2016-12-11 14:10:01]

That seems fine to me as long as we clearly document it.

However, I'm curious why we don't see the same problem with the Linux
implementation. Do we simply not run those tests on Linux?

On Dec 10, 2016 6:35 PM, <lawrencewu@chromium.org> wrote:

> On 2016/12/10 02:05:02, lawrencewu wrote:
> > mark@: I had to readd the POSIX implementation for Duplicate().
> >
> > I thought I didn't need it for my project, but I forgot the unit tests
> still
> use
> > it (e.g.
> >
> https://cs.chromium.org/chromium/src/base/metrics/
> field_trial_unittest.cc?q=field_trial_unittest&sq=
> package:chromium&dr&l=1193).
> > Please take a look, thanks.
>
> asvitkine@: I had to change the method of field trial handle passing on
> POSIX.
>
> The problem with the current method is some browser tests don't initialize
> the
> allocator, but the child process still tries to open the fd.
> Since it can't distinguish anymore whether or not the fd is valid (because
> it's
> a constant), the map will fail. I solved this by passing the
> --field-trial-handle flag and making the child check for the presence of
> the
> flag (the value is invalid but it doesn't matter). Let me know
> if this is acceptable and I'll update the comments and docs to reflect
> this.
>
> https://codereview.chromium.org/2565683003/
>

-- 
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.

[lawrencewu, 2016-12-11 17:58:10]

On 2016/12/11 14:10:01, Alexei Svitkine (slow) wrote:
> That seems fine to me as long as we clearly document it.
> 
> However, I'm curious why we don't see the same problem with the Linux
> implementation. Do we simply not run those tests on Linux?

OK, I added comments and will update the design doc.

As for why the failures are Mac-only: all the tests log some error in
vt_video_encode_accelerator_mac.cc, so I suspect some interaction between
shm in the field trial segment and this is causing these crashes. This might
also be related to crbug.com/672889.

[lawrencewu, 2016-12-11 17:58:46]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 17:59:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 20:07:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 20:07:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lawrencewu, 2016-12-11 20:58:04]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 20:58:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 22:40:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 22:40:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lawrencewu, 2016-12-11 23:23:20]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 23:23:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 00:10:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 00:10:45]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-12-12 16:02:39]

lgtm

https://codereview.chromium.org/2565683003/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2565683003/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:947:
cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);
If in this case you're using the param with an intentionally dummy value, just
pass a clear dummy value here - e.g. a hardcoded string "1" or whatever.

[lawrencewu, 2016-12-12 22:29:51]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 22:33:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 01:07:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 01:07:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-12-13 17:17:58]

The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 17:18:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lawrencewu, 2016-12-13 17:18:40]

Address comments.

mark@: PTAL, thanks.

https://codereview.chromium.org/2565683003/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2565683003/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:947:
cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);
On 2016/12/12 16:02:39, Alexei Svitkine (OO from 15th) wrote:
> If in this case you're using the param with an intentionally dummy value, just
> pass a clear dummy value here - e.g. a hardcoded string "1" or whatever.

Done.

[Mark Mentovai, 2016-12-13 17:55:59]

LGTM

[commit-bot: I haz the power, 2016-12-13 18:23:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 18:23:21]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-12-13 19:50:14]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-12-13 19:50:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2565683003/#ps160001
(title: "Use intentionally dummy value and fix comment")

[commit-bot: I haz the power, 2016-12-13 19:51:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 19:54:37]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481658614612260,
"parent_rev": "eb4733f0c5ab9e192ff5d16726f1199d0ec6f34b", "commit_rev":
"e2275a42ccb549ee7a87efffb32d4ff2b361cdc4"}

[commit-bot: I haz the power, 2016-12-13 19:55:04]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2565683003
==========

[commit-bot: I haz the power, 2016-12-13 19:55:06]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-13 19:59:21]

Description was changed from

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2565683003
==========

to

==========
Share field trial allocator on Mac

This CL shares the field trial allocator on MacOS. It depends on Mac having
POSIX shared memory support, but other than that it is basically the same
mechanism as that in the Linux CL: https://codereview.chromium.org/2530573002/.
We basically just need to edit some ifdefs.

NOTE: turning on NOPRESUBMIT because the dependent CL re-adds some banned calls
to AllowIO.

BUG=663912
NOPRESUBMIT=true

Committed: https://crrev.com/d17b445e932a10c875cea034b037b19d30e47a8c
Cr-Commit-Position: refs/heads/master@{#438265}
==========

[commit-bot: I haz the power, 2016-12-13 19:59:22]

Patchset 9 (id:??) landed as
https://crrev.com/d17b445e932a10c875cea034b037b19d30e47a8c
Cr-Commit-Position: refs/heads/master@{#438265}