New issue
Advanced search Search tips
Starred by 4 users

Issue metadata

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

Blocking:
issue 598073
issue 838964


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

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

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

Sign in to add a comment