New issue
Advanced search Search tips

Issue 900733 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 900705



Sign in to add a comment

Ensure there is no memory bloat when experimental hints are enabled

Project Member Reported by tbansal@chromium.org, Oct 31

Issue description

In the previews hints, if a loading hint is marked as experimental, then we may still load them in the memory.  However, we would not count those hints towards GetMaxPageHintsInMemoryThreshhold(). We should either not load the experimental hints in the memory, or if we do, then we should count them towards the threshold.
 
Owner: jegray@chromium.org
Status: Assigned (was: Untriaged)
Assigning to Jered since it seems Jered's in-flight CL already covers this.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 2

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

commit b40eed3b1e84a9ae7a3e71bc26a694546a6e9775
Author: Jered Gray <jegray@google.com>
Date: Fri Nov 02 00:41:27 2018

Strip disabled optimizations when parsing preview hints

The logic in PreviewsHints::CreateFromConfig() has been modified so that
it now strips out optimizations that are disabled and only allows one of
each optimization type within a PageHint. Duplicate types encountered
are also stripped. This makes it so that enabling an experimental
optimization will automatically disable a subsequent optimization of the
same optimization type in the same PageHint. If all optimizations for a
page hint are stripped, then the page hint is also stripped.

This fixes a bug related to disabled page hint optimizations being
usable, as the disabled optimizations will no longer exist in the
hint cache.

As a result of this change, when the total page hints with resource
hints exceeds the cap, the number of page hints added will always
exactly match the cap. This is due to the logic for stripping hints
allowing us to add hints that only include some of their page hints.

A DCHECK has been added verifying that non-resource loading hint
optimizations never contain resource loading hints.

Additionally, the description of |whitelisted_optimizations| for the
PageHint proto has been updated to match expectations that it will be
provided as an ordered list.

Unittests have been added to verify that the stripping is handled
properly and that the number of page hints allowed stops exactly at the
cap. Some issues with unittests in PreviewsOptimizationGuideUnittest
using InitializeMultipleResourceLoadingHints() have also been fixed.

Bug: 900725,  900733 
Change-Id: Ic8d8ce2de98e4e2993cab994906c3b2e43c31a22
Reviewed-on: https://chromium-review.googlesource.com/c/1311796
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Jered Gray <jegray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604773}
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/optimization_guide/proto/hints.proto
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/previews/content/previews_hints.cc
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/previews/content/previews_hints.h
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/previews/content/previews_hints_unittest.cc
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/previews/content/previews_optimization_guide.cc
[modify] https://crrev.com/b40eed3b1e84a9ae7a3e71bc26a694546a6e9775/components/previews/content/previews_optimization_guide_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 5

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afae6078bb535945258432310aa485e910a8d171

commit afae6078bb535945258432310aa485e910a8d171
Author: Jered Gray <jegray@google.com>
Date: Mon Nov 05 20:39:34 2018

Strip disabled optimizations when parsing preview hints

Merge to release branch

The logic in PreviewsHints::CreateFromConfig() has been modified so that
it now strips out optimizations that are disabled and only allows one of
each optimization type within a PageHint. Duplicate types encountered
are also stripped. This makes it so that enabling an experimental
optimization will automatically disable a subsequent optimization of the
same optimization type in the same PageHint. If all optimizations for a
page hint are stripped, then the page hint is also stripped.

This fixes a bug related to disabled page hint optimizations being
usable, as the disabled optimizations will no longer exist in the
hint cache.

As a result of this change, when the total page hints with resource
hints exceeds the cap, the number of page hints added will always
exactly match the cap. This is due to the logic for stripping hints
allowing us to add hints that only include some of their page hints.

A DCHECK has been added verifying that non-resource loading hint
optimizations never contain resource loading hints.

Additionally, the description of |whitelisted_optimizations| for the
PageHint proto has been updated to match expectations that it will be
provided as an ordered list.

Unittests have been added to verify that the stripping is handled
properly and that the number of page hints allowed stops exactly at the
cap. Some issues with unittests in PreviewsOptimizationGuideUnittest
using InitializeMultipleResourceLoadingHints() have also been fixed.

Bug: 900725,  900733 
Change-Id: Ic8d8ce2de98e4e2993cab994906c3b2e43c31a22
Reviewed-on: https://chromium-review.googlesource.com/c/1311796
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Jered Gray <jegray@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604773}
Reviewed-on: https://chromium-review.googlesource.com/c/1318534
Cr-Commit-Position: refs/branch-heads/3578@{#522}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/optimization_guide/proto/hints.proto
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/previews/content/previews_hints.cc
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/previews/content/previews_hints.h
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/previews/content/previews_hints_unittest.cc
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/previews/content/previews_optimization_guide.cc
[modify] https://crrev.com/afae6078bb535945258432310aa485e910a8d171/components/previews/content/previews_optimization_guide_unittest.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/afae6078bb535945258432310aa485e910a8d171

Commit: afae6078bb535945258432310aa485e910a8d171
Author: jegray@google.com
Commiter: tbansal@chromium.org
Date: 2018-11-05 20:39:34 +0000 UTC

Strip disabled optimizations when parsing preview hints

Merge to release branch

The logic in PreviewsHints::CreateFromConfig() has been modified so that
it now strips out optimizations that are disabled and only allows one of
each optimization type within a PageHint. Duplicate types encountered
are also stripped. This makes it so that enabling an experimental
optimization will automatically disable a subsequent optimization of the
same optimization type in the same PageHint. If all optimizations for a
page hint are stripped, then the page hint is also stripped.

This fixes a bug related to disabled page hint optimizations being
usable, as the disabled optimizations will no longer exist in the
hint cache.

As a result of this change, when the total page hints with resource
hints exceeds the cap, the number of page hints added will always
exactly match the cap. This is due to the logic for stripping hints
allowing us to add hints that only include some of their page hints.

A DCHECK has been added verifying that non-resource loading hint
optimizations never contain resource loading hints.

Additionally, the description of |whitelisted_optimizations| for the
PageHint proto has been updated to match expectations that it will be
provided as an ordered list.

Unittests have been added to verify that the stripping is handled
properly and that the number of page hints allowed stops exactly at the
cap. Some issues with unittests in PreviewsOptimizationGuideUnittest
using InitializeMultipleResourceLoadingHints() have also been fixed.

Bug: 900725,  900733 
Change-Id: Ic8d8ce2de98e4e2993cab994906c3b2e43c31a22
Reviewed-on: https://chromium-review.googlesource.com/c/1311796
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Jered Gray <jegray@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604773}
Reviewed-on: https://chromium-review.googlesource.com/c/1318534
Cr-Commit-Position: refs/branch-heads/3578@{#522}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Assigned)
Labels: -M-72 M-71

Sign in to add a comment