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

Issue 789657 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 598073
issue 853798
issue 838964
issue 844918


Show other hotlists

Hotlists containing this issue:
XXX


Sign in to add a comment

Figure out what to do about MediaURLRequestContexts when the network service is enabled.

Project Member Reported by mmenke@chromium.org, Nov 29 2017

Issue description

They exist solely to have their own disk cache, and are otherwise identical to the main URLRequestContext.  Ideally, we could just get rid of them (And maybe increase the size of the now shared cache a bit to compensate, or maybe improve handling of large files?)
 
Cc: hubbe@chromium.org
Cc: -hubbe@chromium.org
Owner: hubbe@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by hubbe@chromium.org, Jan 25 2018

Cc: hubbe@chromium.org
Owner: mmenke@chromium.org

Comment 4 by mmenke@chromium.org, Jan 25 2018

Owner: ----
Status: Available (was: Assigned)
Not sure why you're assigning this one to me.

Comment 5 by hubbe@chromium.org, Jan 25 2018

I didn't have any context for this bug, and figured it has been assigned to me by mistake.

What is the "the network service" ?


Comment 6 by mmenke@chromium.org, Jan 25 2018

I can't explain why yiningc's assigned the bug to you, but the network service involves running network code out of process, as a mojo service.  An API that allows creating two NetworkContexts (The new top-level network object, used by consumers in the browser process) that share everything but a cache just seems much too ugly, and the dual cache decision was pretty hacky in the first place.

Comment 7 by hubbe@chromium.org, Jan 25 2018

Not sure how that makes any difference to media though, I mean network stuff is always handled by a different process, it just used to be the browser. So what is this "dual cache decision", and who made it? (can we assign the bug to them?)

Comment 8 by mmenke@chromium.org, Jan 25 2018

Extremely unlikely - it's been this way for quite a while.  Looks like it was done by hclam, back in 2009?  http://codereview.chromium.org/19747.  I suspect he was on the media team, but I'm not positive of that.  He was not on the network team.

Regardless, I'm not sure why we need to assign the bug at this point?
Yes he was long ago on media team; I'd support any experiment turning these off and watching impact on our metrics.

Comment 10 by hubbe@chromium.org, Jan 25 2018

I agree with Dale.

Cc: morlovich@chromium.org
+morlovich@: Do you know if there's anything special with net::MEDIA_CACHE? 

I did a code search, and net::MEDIA_CACHE cache type doesn't seem to do any optimization. The special handling code (added in https://codereview.chromium.org/19747/) seem to be gone.

We could start by removing net::MEDIA_CACHE cache type first (before running a 
field trial to combine media URLRequestContexts with main URLRequestContexts). 
I'm pretty net::MEDIA_CACHE is just for histograms, though defer to Maks on anything cache-related.
The big thing is the two separate directories, so video files don't end up kicking out everything else.

As for other effects:
The blockfile cache uses totally different eviction algorithm for the main cache than everything else, with media cache not included. Not sure if that's intentional.

Also looks like optimistic ops for simple are off for media cache which... is pretty much a bug. Woops.

(Those both check for net::DISK_CACHE rather than net::MEDIA_CACHE).


Blocking: 838964
Labels: -Pri-3 Proj-Servicification-Canary Pri-1
Project Member

Comment 16 by bugdroid1@chromium.org, May 17 2018

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

commit 3f0d18e0cfc5c485c743631c8d55737e559e7cd4
Author: Matt Menke <mmenke@chromium.org>
Date: Thu May 17 22:33:51 2018

Add an experimental "feature" to disable the media cache.

When enabled, a Profile's media URLRequestContext will just use the
same disk cache as the main one. Also affect App Media
URLRequestContexts.

Bug: 789657
Change-Id: I4150324ad84c1679495bd512dd0231edb30c733b
Reviewed-on: https://chromium-review.googlesource.com/1064858
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559712}
[modify] https://crrev.com/3f0d18e0cfc5c485c743631c8d55737e559e7cd4/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/3f0d18e0cfc5c485c743631c8d55737e559e7cd4/chrome/browser/profiles/profile_impl_io_data.cc
[modify] https://crrev.com/3f0d18e0cfc5c485c743631c8d55737e559e7cd4/chrome/common/chrome_features.cc
[modify] https://crrev.com/3f0d18e0cfc5c485c743631c8d55737e559e7cd4/chrome/common/chrome_features.h

Comment 17 by dxie@chromium.org, May 18 2018

Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: mmenke@chromium.org
Status: Assigned (was: Available)
Status: Started (was: Assigned)
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 6

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

commit b0f4c7ce78b25c620cd3bd3253520cdabcb63ee9
Author: Matt Menke <mmenke@chromium.org>
Date: Fri Jul 06 19:25:30 2018

Add a "feature" to scale the disk cache size.

This is for use with the same disk cache for media experiment.

Bug: 789657
Change-Id: I3374166de4811df85b6c38e8988fee42a609f96f
Reviewed-on: https://chromium-review.googlesource.com/1124764
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573040}
[modify] https://crrev.com/b0f4c7ce78b25c620cd3bd3253520cdabcb63ee9/net/disk_cache/cache_util.cc
[modify] https://crrev.com/b0f4c7ce78b25c620cd3bd3253520cdabcb63ee9/net/disk_cache/cache_util.h
[modify] https://crrev.com/b0f4c7ce78b25c620cd3bd3253520cdabcb63ee9/net/disk_cache/cache_util_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 11

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

commit e226149af1b3f003b5d3f59e19ee17bef7419c42
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Jul 11 20:05:30 2018

Delete the media cache(s) when using a single combined HTTP cache.

This is so that I can try increasing the media cache size in experiments
without increasing Chrome's profile size (Except possibly for profiles
that have never been used to play video).

Bug: 789657
Change-Id: I45e8f8c6e346a1fa7d05b0bea3e8b001d7dc41e1
Reviewed-on: https://chromium-review.googlesource.com/1130162
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574296}
[modify] https://crrev.com/e226149af1b3f003b5d3f59e19ee17bef7419c42/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/e226149af1b3f003b5d3f59e19ee17bef7419c42/chrome/browser/profiles/profile_impl_io_data.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Jul 12

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

commit eb81f73d09c9efd4ff7eb181ce3ffe105d1549a8
Author: Mike West <mkwst@chromium.org>
Date: Thu Jul 12 09:35:30 2018

Revert "Delete the media cache(s) when using a single combined HTTP cache."

This reverts commit e226149af1b3f003b5d3f59e19ee17bef7419c42.

Reason for revert: 

`ProfileWithoutMediaCacheBrowserTest.DeleteMediaCache` and `ProfileWithoutMediaCacheBrowserTest.DeleteIsolatedAppMediaCache` have been failing, apparently since they were added by this CL.

They were added here: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.11%20Tests/28016 (which failed to actually run `browser_test`) and have been failing consistently since https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.11%20Tests/28018.


Original change's description:
> Delete the media cache(s) when using a single combined HTTP cache.
> 
> This is so that I can try increasing the media cache size in experiments
> without increasing Chrome's profile size (Except possibly for profiles
> that have never been used to play video).
> 
> Bug: 789657
> Change-Id: I45e8f8c6e346a1fa7d05b0bea3e8b001d7dc41e1
> Reviewed-on: https://chromium-review.googlesource.com/1130162
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Commit-Queue: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574296}

TBR=mmenke@chromium.org,morlovich@chromium.org

Change-Id: Ibc519750f00ad7e698d279af6baffe77b32d30be
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 789657,  862945 
Reviewed-on: https://chromium-review.googlesource.com/1134886
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574513}
[modify] https://crrev.com/eb81f73d09c9efd4ff7eb181ce3ffe105d1549a8/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/eb81f73d09c9efd4ff7eb181ce3ffe105d1549a8/chrome/browser/profiles/profile_impl_io_data.cc

Labels: M-70
Project Member

Comment 24 by bugdroid1@chromium.org, Jul 24

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

commit 685baf97d7a8e6c43e2f96f43f8bf5b7f78cb586
Author: Matt Menke <mmenke@chromium.org>
Date: Tue Jul 24 17:27:58 2018

Delete the media cache(s) when using a single combined HTTP cache.

This is so that I can try increasing the media cache size in experiments
without increasing Chrome's profile size (Except possibly for profiles
that have never been used to play video).

This is a relant of
https://chromium-review.googlesource.com/c/chromium/src/+/1130162, with
re-worked tests.

Bug: 789657
Change-Id: I057e2e657572e21732610aefc468550e58fabf77
Reviewed-on: https://chromium-review.googlesource.com/1145137
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577595}
[modify] https://crrev.com/685baf97d7a8e6c43e2f96f43f8bf5b7f78cb586/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/685baf97d7a8e6c43e2f96f43f8bf5b7f78cb586/chrome/browser/profiles/profile_impl_io_data.cc

matt, has this been resolved? let me know what's remaining, thanks!
No, it has not - need to launch a new field trial, and see how much of a difference a bigger cache with a unified cache makes with perf.  Was going to do it this week, but everyone's out, and am sufficiently paranoid about dealing with multiple features in a single field trial that I decided not to launch it while Maks is out.
Labels: -Proj-Servicification-Canary Proj-Servicification
I think we can probably go to Canary without this, though, so removing the Canary label, though it will make perf comparisons less meaningful.
Labels: Hotlist-KnownIssue
Blocking: 844918
Blocking: 853798
Hey Matt, can you provide an update on this? Has nothing happened in 2 months or is there stuff that's not captured here? Thanks
I ran the trial for a month and a half or so with legacy behavior/merged cache/merged cache of 1.5x size/merged cache of 2x size.

Results were *extremely* noisy, to the extent that I don't have a whole lot of faith in even a months worth of aggregated data.  The network TimeToFirstByte and TotalTime (for request) histograms pretty clearly regressed a bit for all 3 experimental groups.  The cache hit rates seem to either barely improve or be about the same.  NavigationToFirstContentFul paint may have just slightly improved in some cases, worse in others...Honestly, given inconsistencies between groups (There's a bias of whose in a network service enabled vs no network service group vs network service enabled, which really isn't compatible with looking at a month's worth of data from this experiment), there's not a whole lot more I can say.
Also, as I said, the data is really, really noisy.  Data from one week to the next makes it difficult to figure out if there's a trend, other than network times definitely regression slightly, in aggregate.
FYI you can exclude users in network service trial in finch, e.g. https://uma.googleplex.com/p/chrome/variations?sid=79a8f62ae910ab261175561d917fa725

However it's still noisy. Generally perf numbers are only trusted in stable, did you consider running this experiment on a subset of stable users?
The requisite code isn't on stable, and don't stable trials require a lot of extra overhead (launch bug, design doc, etc?)
Oh, and as I said - there are significant differences between "Control" and "Not in trial" numbers for the NetworkService trial, which may be noise...or may be population biases.
It seems that only the last check-in (deleting the media cache) isn't in 69. If I'm reading Quota.AvailableDiskSpace correctly, the median Windows user has > 167GB free so it seems fine that we would use more profile space for users in this experiment.

Yes, stable experiments have a bunch of requirements per https://docs.google.com/document/d/1hJ1U8-7DNa7lGfTJWRgSgqQyNnOFO4Ks5Czr1-3--8I/edit#heading=h.ja0j8vjwrlfs. At the same time, afaik this is the only trusted channel for performance comparisons because the others users' are considered too biased.

Given that we don't have conclusive data so far for the media cache experiments, and we will start m70 stable trials next week for network-service at low percentages, the no-separate-media-cache can be considered as one of the (many) internal behavioral changes of network service. This does mean we don't have insight into what portion of performance difference, if any, are related to the combining of caches vs other changes.
Cc: mmenke@chromium.org
Owner: ----
Status: Available (was: Started)
Unassigning myself from network service issues.
Labels: Enterprise-Triaged

Comment 40 by dbbrooks@chromium.org, Jan 15 (3 days ago)

Owner: morlovich@chromium.org
Status: Assigned (was: Available)
Hi, I'm doing some triage of bugs under Internals>Media. Is it OK to have a P1 bug without an owner? 

In case not, I pinged morlovich@ who graciously accepted the honor. Thanks!

Comment 41 by morlovich@chromium.org, Jan 16 (2 days ago)

Labels: -Pri-1 Pri-2

Sign in to add a comment