New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 867558 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-11-15
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocked on:
issue 752368



Sign in to add a comment

Get rid of fallback code path of synchronizing field trial state via command line instead of shmem

Project Member Reported by asvitk...@chromium.org, Jul 25

Issue description

Get stats on how many clients are falling back to synchronizing field trial state via command line instead of shmem.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9372cdbb69402431465f34333d0990b76c31927e

commit 9372cdbb69402431465f34333d0990b76c31927e
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Wed Jul 25 20:22:06 2018

Instrument child process field trial creation to measure success.

This will let us see how many clients are falling back to using
the command-line to synchronize state and also whether any of
them are failing to create field trials from shared memory or
command line.

Bug: 867558
Change-Id: If1dd1f694686a98b67a553344f3a3e2289424430
Reviewed-on: https://chromium-review.googlesource.com/1150505
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578036}
[modify] https://crrev.com/9372cdbb69402431465f34333d0990b76c31927e/base/metrics/field_trial.cc
[modify] https://crrev.com/9372cdbb69402431465f34333d0990b76c31927e/tools/metrics/histograms/histograms.xml

Initial canary data shows it always goes through shared memory and always successful.

This is promising. If this is the case for other channels, we can clean up the fall back logic.
NextAction: 2018-10-15
Above CL landed in M70 so need to wait for M70 stable before can get stats on stable.
The NextAction date has arrived: 2018-10-15
The NextAction date has arrived: 2018-11-15
Issue 899963 has been merged into this issue.
So ChildProcess.FieldTrials.CreateFromSwitchSuccess shows no values logged:

https://uma.googleplex.com/p/chrome/timeline_v2/?sid=eff4449fa1cfb75fbcd65cd6ac1f7845

ChildProcess.FieldTrials.CreateFromShmemSuccess shows very low proportion for failures - 0 on Mac and very tiny on Windows.


For the cases where ChildProcess.FieldTrials.CreateFromShmemSuccess recorded a failure there was no kForceFieldTrials switch and therefore the fallback code path wasn't taken. So we can remove that code path without any issues.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4428eafac7163d24267ab56c812cbedb977e66d8

commit 4428eafac7163d24267ab56c812cbedb977e66d8
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Wed Nov 21 05:46:27 2018

[Cleanup] Remove kUseSharedMemoryForFieldTrials in field_trial.c.

This constant was only set to false on OS_FUCHSIA and has been
shipping as true for several years on all other platforms.

Cleans up the code to not need this constant in most places and
adds a OS_FUCHSIA ifdef around InstantiateFieldTrialAllocatorIfNeeded()
which is the only place that needed custom logic to disable it for
Fuchsia.

Bug: 867558
Change-Id: Idbf49ce571a50d67debeafe2cdacee086ebca537
Reviewed-on: https://chromium-review.googlesource.com/c/1343020
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609922}
[modify] https://crrev.com/4428eafac7163d24267ab56c812cbedb977e66d8/base/metrics/field_trial.cc

Cc: w...@chromium.org
So the only thing that relies on the fallback pack is Fuchsia. But it also doesn't sent histograms - so our measurements of this are not useful.

Ideally, Fuchsia can just move over to shared memory and then we can kill the codepath. Not sure what's blocking that. For example, some tests that are already running and passing on Fuchsia exercise the shared memory function (e.g. InstantiateFieldTrialAllocatorIfNeeded()). So not sure what blocks us from enabling it fully...

Adding wez@ who may have some insight here.
fallback *path
I guess the missing bit is the logic to share the shared memory handle with the child process?

i.e. I don't see the relevant bits in child_process_launcher_helper_fuchsia.cc.

I think for the purposes of this bug, we can disable the fallback logic on non-Fuchsia platforms per the histograms that indicate it's never triggered and remove those histograms.
Re #11-13: We filed issue 752368 for this and just didn't get around to implementing it since it hasn't been a priority up until now.  We'll prioritize getting it implemented so the old path can be cleaned-up, though.
Blockedon: 752368
Labels: -Pri-2 OS-Fuchsia Pri-3
Summary: Get rid of fallback code path of synchronizing field trial state via command line instead of shmem (was: Get stats on how many clients are falling back to synchronizing field trial state via command line instead of shmem)
Retitling this bug to reflect that the next step is to remove the command-line codepath and marking as blocked on the fuchsia bug.
FWIW Fuchsia is blocked on a refactoring of the handle-passing logic to
follow the pattern used by e.g. Mojo. (Under Fuchsia we generate the
command-line serialised handle Id when we add the handle to the
HandlesToTransfer vector)

If no-one else gets to that first then I'll take care of it in the new year.

On Mon, 17 Dec 2018, 17:56 asvitkine via monorail <
monorail+v2.3492304296@chromium.org wrote:

Sign in to add a comment