Get rid of fallback code path of synchronizing field trial state via command line instead of shmem |
|||||
Issue descriptionGet stats on how many clients are falling back to synchronizing field trial state via command line instead of shmem.
,
Jul 26
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.
,
Sep 25
Above CL landed in M70 so need to wait for M70 stable before can get stats on stable.
,
Oct 15
The NextAction date has arrived: 2018-10-15
,
Oct 15
Don't have stable data yet: https://uma.googleplex.com/p/chrome/timeline_v2/?sid=37330c33bee2cdb60bcb8c4b8a3cf993
,
Nov 15
The NextAction date has arrived: 2018-11-15
,
Nov 19
Issue 899963 has been merged into this issue.
,
Nov 19
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.
,
Nov 19
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.
,
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
,
Nov 21
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.
,
Nov 21
fallback *path
,
Nov 21
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.
,
Nov 27
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.
,
Dec 17
Retitling this bug to reflect that the next step is to remove the command-line codepath and marking as blocked on the fuchsia bug.
,
Dec 18
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 |
|||||
Comment 1 by bugdroid1@chromium.org
, Jul 25