Skip to content

Commit

Permalink
Remove obsolete CachedImagesCount.{Software, Gpu} histogram.
Browse files Browse the repository at this point in the history
Because they are recorded in the destructors for the caches, which are
very rarely (if ever) called, these have not reported metrics for quite
some time. This change removes the calls to emit the histograms and
marks them as obsolete in histograms.xml.

Per crbug.com/963619, we will replace this with a more active metric
that incorporates percentage of memory used by the cached images.

Bug: 963619
Change-Id: Ib780ab15b1e78ed579da714105fd68829536546b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1613719
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Madeleine Barowsky <mbarowsky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660481}
  • Loading branch information
Madeleine Barowsky authored and Commit Bot committed May 16, 2019
1 parent 2a344e6 commit aceeb52
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 33 deletions.
14 changes: 0 additions & 14 deletions cc/tiles/gpu_image_decode_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -891,17 +891,6 @@ GpuImageDecodeCache::~GpuImageDecodeCache() {
// It is safe to unregister, even if we didn't register in the constructor.
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
this);

// TODO(vmpstr): If we don't have a client name, it may cause problems in
// unittests, since most tests don't set the name but some do. The UMA system
// expects the name to be always the same. This assertion is violated in the
// tests that do set the name.
if (GetClientNameForMetrics()) {
UMA_HISTOGRAM_CUSTOM_COUNTS(
base::StringPrintf("Compositing.%s.CachedImagesCount.Gpu",
GetClientNameForMetrics()),
lifetime_max_items_in_cache_, 1, 1000, 20);
}
}

ImageDecodeCache::TaskResult GpuImageDecodeCache::GetTaskForImageAndRef(
Expand Down Expand Up @@ -1593,9 +1582,6 @@ bool GpuImageDecodeCache::EnsureCapacity(size_t required_size) {
"GpuImageDecodeCache::EnsureCapacity");
lock_.AssertAcquired();

lifetime_max_items_in_cache_ =
std::max(lifetime_max_items_in_cache_, persistent_cache_.size());

// While we are over preferred item capacity, we iterate through our set of
// cached image data in LRU order, removing unreferenced images.
for (auto it = persistent_cache_.rbegin();
Expand Down
4 changes: 0 additions & 4 deletions cc/tiles/gpu_image_decode_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -665,10 +665,6 @@ class CC_EXPORT GpuImageDecodeCache
std::vector<uint32_t> ids_pending_unlock_;
std::vector<uint32_t> ids_pending_deletion_;

// Records the maximum number of items in the cache over the lifetime of the
// cache. This is updated anytime we are requested to reduce cache usage.
size_t lifetime_max_items_in_cache_ = 0u;

std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
};

Expand Down
12 changes: 0 additions & 12 deletions cc/tiles/software_image_decode_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,6 @@ SoftwareImageDecodeCache::~SoftwareImageDecodeCache() {
// It is safe to unregister, even if we didn't register in the constructor.
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
this);
// TODO(vmpstr): If we don't have a client name, it may cause problems in
// unittests, since most tests don't set the name but some do. The UMA system
// expects the name to be always the same. This assertion is violated in the
// tests that do set the name.
if (GetClientNameForMetrics()) {
UMA_HISTOGRAM_CUSTOM_COUNTS(
base::StringPrintf("Compositing.%s.CachedImagesCount.Software",
GetClientNameForMetrics()),
lifetime_max_items_in_cache_, 1, 1000, 20);
}
}

ImageDecodeCache::TaskResult SoftwareImageDecodeCache::GetTaskForImageAndRef(
Expand Down Expand Up @@ -584,8 +574,6 @@ void SoftwareImageDecodeCache::DrawWithImageFinished(
void SoftwareImageDecodeCache::ReduceCacheUsageUntilWithinLimit(size_t limit) {
TRACE_EVENT0("cc",
"SoftwareImageDecodeCache::ReduceCacheUsageUntilWithinLimit");
lifetime_max_items_in_cache_ =
std::max(lifetime_max_items_in_cache_, decoded_images_.size());
for (auto it = decoded_images_.rbegin();
decoded_images_.size() > limit && it != decoded_images_.rend();) {
if (it->second->ref_count != 0) {
Expand Down
3 changes: 0 additions & 3 deletions cc/tiles/software_image_decode_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,6 @@ class CC_EXPORT SoftwareImageDecodeCache
const PaintImage::GeneratorClientId generator_client_id_;

size_t max_items_in_cache_;
// Records the maximum number of items in the cache over the lifetime of the
// cache. This is updated anytime we are requested to reduce cache usage.
size_t lifetime_max_items_in_cache_ = 0u;
};

} // namespace cc
Expand Down
10 changes: 10 additions & 0 deletions tools/metrics/histograms/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17667,6 +17667,11 @@ uploading your change for review.
</histogram>

<histogram name="Compositing.Browser.CachedImagesCount" units="count">
<obsolete>
Deprecated 05/2019 because it no longer reports. To be superseded by a
measurement related to percentage of cache/discardable memory used by image
caching.
</obsolete>
<owner>vmpstr@chromium.org</owner>
<summary>
The maximum number of images that were cached in the browser over the
Expand Down Expand Up @@ -18170,6 +18175,11 @@ uploading your change for review.
</histogram>

<histogram name="Compositing.Renderer.CachedImagesCount" units="count">
<obsolete>
Deprecated 05/2019 because it no longer reports. To be superseded by a
measurement related to percentage of cache/discardable memory used by image
caching.
</obsolete>
<owner>vmpstr@chromium.org</owner>
<summary>
The maximum number of images that were cached in the renderer over the
Expand Down

0 comments on commit aceeb52

Please sign in to comment.