New issue
Advanced search Search tips

Issue 908985 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

OptimizationGuideService makes unnecessary copies of proto::Configuration

Project Member Reported by jegray@chromium.org, Nov 27

Issue description

The logic in OptimizationGuideService creates an
optimization_guide::proto::Configuration object on a background thread
and then uses task posting to send it on to
PreviewsHints::CreateFromConfig(), where the object is used to create a
PreviewsHints object on another background thread. In all, there are
two tasks posted that included the config object as a parameter, one on
the background thread and one on the UI thread. Including it as a
parameter in a task requires a full copy of the protobuffer. This means
that a full copy of the config protobuf is occurring twice, once on the UI thread.

This should be eliminated so that we don't take the performance hit from the unnecessary copies.

 
Components: Blink>Previews
Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 29

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

commit 1b420fdb7ff25f00b37319031d243424b1db2b0e
Author: Jered Gray <jegray@google.com>
Date: Thu Nov 29 01:36:31 2018

Eliminate unnecessary proto::Configuration copies

The previous logic in OptimizationGuideService created an
optimization_guide::proto::Configuration object on a background thread
and then used task posting to send it on to
PreviewsHints::CreateFromConfig(), where it was used to create a
PreviewsHints object on another background thread. In all, there were
two tasks posted that included the config object as a parameter, one on
the background thread and one on the UI thread. Including it as a
parameter in a task requires a full copy of the protobuffer. This means
that a full copy of the config protobuf, which can be as large as 1.8MB
(the size of the current Brazil one on Canary), was occurring on the UI
thread.

In local performance measurements, making a single copy of a 600KB
version of the protobuf took several milliseconds (it amounted to
roughly 60% of the time taken by the initial component string parsing
and 60% of the time taken by PreviewHints::CreateFromConfig()). Given
that we were incurring the cost of two copies, one of which was on an
extremely high priority thread, it makes sense to change the logic to
eliminate the need for the copies.

The logic has been altered so that OptimizationGuideService no longer
immediately processes the component, but instead notifies the
observers that it is available and allows them to trigger the
processing. This eliminates both copies of the configuration protobuf,
as it is now created where it is used.

Additionally, OptimizationGuideServiceObservers are now immediately
notified of the hints component when they register if one is already
available. This will enable the PreviewsOptimizationGuide to wait until
the HintCacheLevelDBStore is fully initialized before registering for
the component, and in the future will potentially allow it to avoid
processing the configuration altogether when the store already has the
latest version cached.

New unittests have been added and older ones have been updated to
accommodate the new logic.

The related browser tests have also been modified to be more robust,
where they now wait for a signal from a local histogram indicating
that hint processing is complete.

Bug:  908985 

Change-Id: I330fcc35f14388b8a704d3180bd758e7327e9892
Reviewed-on: https://chromium-review.googlesource.com/c/1351719
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611982}
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/chrome/browser/component_updater/optimization_hints_component_installer_unittest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/chrome/browser/previews/previews_browsertest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/chrome/browser/previews/resource_loading_hints/resource_loading_hints_browsertest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/BUILD.gn
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/hints_component_info.h
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/hints_component_util.cc
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/hints_component_util.h
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/hints_component_util_unittest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/optimization_guide_service.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/optimization_guide_service.h
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/optimization_guide_service_observer.h
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/optimization_guide_service_unittest.cc
[rename] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/test_hints_component_creator.cc
[rename] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/optimization_guide/test_hints_component_creator.h
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_hints.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_hints.h
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_hints_unittest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_optimization_guide.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_optimization_guide.h
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/content/previews_optimization_guide_unittest.cc
[modify] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/core/BUILD.gn
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/core/previews_constants.cc
[add] https://crrev.com/1b420fdb7ff25f00b37319031d243424b1db2b0e/components/previews/core/previews_constants.h

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 29

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

commit e9de970cf577dc29b86d81752230b4ef7c1631a4
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Nov 29 05:02:40 2018

Revert "Eliminate unnecessary proto::Configuration copies"

This reverts commit 1b420fdb7ff25f00b37319031d243424b1db2b0e.

Reason for revert: Makes PreviewsNoScriptBrowserTest and 
ResourceLoadingHintsBrowserTest flaky

 https://crbug.com/909998 
 https://crbug.com/909999 


Original change's description:
> Eliminate unnecessary proto::Configuration copies
> 
> The previous logic in OptimizationGuideService created an
> optimization_guide::proto::Configuration object on a background thread
> and then used task posting to send it on to
> PreviewsHints::CreateFromConfig(), where it was used to create a
> PreviewsHints object on another background thread. In all, there were
> two tasks posted that included the config object as a parameter, one on
> the background thread and one on the UI thread. Including it as a
> parameter in a task requires a full copy of the protobuffer. This means
> that a full copy of the config protobuf, which can be as large as 1.8MB
> (the size of the current Brazil one on Canary), was occurring on the UI
> thread.
> 
> In local performance measurements, making a single copy of a 600KB
> version of the protobuf took several milliseconds (it amounted to
> roughly 60% of the time taken by the initial component string parsing
> and 60% of the time taken by PreviewHints::CreateFromConfig()). Given
> that we were incurring the cost of two copies, one of which was on an
> extremely high priority thread, it makes sense to change the logic to
> eliminate the need for the copies.
> 
> The logic has been altered so that OptimizationGuideService no longer
> immediately processes the component, but instead notifies the
> observers that it is available and allows them to trigger the
> processing. This eliminates both copies of the configuration protobuf,
> as it is now created where it is used.
> 
> Additionally, OptimizationGuideServiceObservers are now immediately
> notified of the hints component when they register if one is already
> available. This will enable the PreviewsOptimizationGuide to wait until
> the HintCacheLevelDBStore is fully initialized before registering for
> the component, and in the future will potentially allow it to avoid
> processing the configuration altogether when the store already has the
> latest version cached.
> 
> New unittests have been added and older ones have been updated to
> accommodate the new logic.
> 
> The related browser tests have also been modified to be more robust,
> where they now wait for a signal from a local histogram indicating
> that hint processing is complete.
> 
> Bug:  908985 
> 
> Change-Id: I330fcc35f14388b8a704d3180bd758e7327e9892
> Reviewed-on: https://chromium-review.googlesource.com/c/1351719
> Commit-Queue: Jered Gray <jegray@chromium.org>
> Reviewed-by: Doug Arnett <dougarnett@chromium.org>
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#611982}

TBR=waffles@chromium.org,tbansal@chromium.org,dougarnett@chromium.org,ryansturm@chromium.org,jegray@chromium.org

Change-Id: I65e2ffff8737cc89bcc0726b4c3e67271e654c2d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  908985 
Reviewed-on: https://chromium-review.googlesource.com/c/1354745
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612044}
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/chrome/browser/component_updater/optimization_hints_component_installer_unittest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/chrome/browser/previews/previews_browsertest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/chrome/browser/previews/resource_loading_hints/resource_loading_hints_browsertest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/BUILD.gn
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/optimization_guide/hints_component_info.h
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/optimization_guide/hints_component_util.cc
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/optimization_guide/hints_component_util.h
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/optimization_guide/hints_component_util_unittest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/optimization_guide_service.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/optimization_guide_service.h
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/optimization_guide_service_observer.h
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/optimization_guide_service_unittest.cc
[rename] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/test_component_creator.cc
[rename] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/optimization_guide/test_component_creator.h
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_hints.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_hints.h
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_hints_unittest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_optimization_guide.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_optimization_guide.h
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/content/previews_optimization_guide_unittest.cc
[modify] https://crrev.com/e9de970cf577dc29b86d81752230b4ef7c1631a4/components/previews/core/BUILD.gn
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/previews/core/previews_constants.cc
[delete] https://crrev.com/092d94af04477cfa9066fb3f0cd757e5bd13c3e6/components/previews/core/previews_constants.h

Labels: -Pri-3 M-72 Pri-2
Status: Started (was: Fixed)
Reopening since the CL got reverted.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 29

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

commit e72337f49c13f7fd4e874ae0c1a38fb74ba69675
Author: Jered Gray <jegray@google.com>
Date: Thu Nov 29 22:26:57 2018

Reland "Eliminate unnecessary proto::Configuration copies"

This is a reland of 1b420fdb7ff25f00b37319031d243424b1db2b0e

The flakiness in the original CL with ResourceLoadingHintsBrowserTest
and PreviewsNoScriptBrowserTest has been fixed.
RetryForHistogramUntilCountReached() now flushes the task scheduler
and keeps trying until the test times out.

Original change's description:
> Eliminate unnecessary proto::Configuration copies
>
> The previous logic in OptimizationGuideService created an
> optimization_guide::proto::Configuration object on a background thread
> and then used task posting to send it on to
> PreviewsHints::CreateFromConfig(), where it was used to create a
> PreviewsHints object on another background thread. In all, there were
> two tasks posted that included the config object as a parameter, one on
> the background thread and one on the UI thread. Including it as a
> parameter in a task requires a full copy of the protobuffer. This means
> that a full copy of the config protobuf, which can be as large as 1.8MB
> (the size of the current Brazil one on Canary), was occurring on the UI
> thread.
>
> In local performance measurements, making a single copy of a 600KB
> version of the protobuf took several milliseconds (it amounted to
> roughly 60% of the time taken by the initial component string parsing
> and 60% of the time taken by PreviewHints::CreateFromConfig()). Given
> that we were incurring the cost of two copies, one of which was on an
> extremely high priority thread, it makes sense to change the logic to
> eliminate the need for the copies.
>
> The logic has been altered so that OptimizationGuideService no longer
> immediately processes the component, but instead notifies the
> observers that it is available and allows them to trigger the
> processing. This eliminates both copies of the configuration protobuf,
> as it is now created where it is used.
>
> Additionally, OptimizationGuideServiceObservers are now immediately
> notified of the hints component when they register if one is already
> available. This will enable the PreviewsOptimizationGuide to wait until
> the HintCacheLevelDBStore is fully initialized before registering for
> the component, and in the future will potentially allow it to avoid
> processing the configuration altogether when the store already has the
> latest version cached.
>
> New unittests have been added and older ones have been updated to
> accommodate the new logic.
>
> The related browser tests have also been modified to be more robust,
> where they now wait for a signal from a local histogram indicating
> that hint processing is complete.

Bug:  908985 ,  910251 
Change-Id: I90407db4c19dac29e10f756a6de87294a9ab683b
Reviewed-on: https://chromium-review.googlesource.com/c/1355256
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612408}
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/chrome/browser/component_updater/optimization_hints_component_installer_unittest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/chrome/browser/previews/previews_browsertest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/chrome/browser/previews/resource_loading_hints/resource_loading_hints_browsertest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/BUILD.gn
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/hints_component_info.h
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/hints_component_util.cc
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/hints_component_util.h
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/hints_component_util_unittest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/optimization_guide_service.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/optimization_guide_service.h
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/optimization_guide_service_observer.h
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/optimization_guide_service_unittest.cc
[rename] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/test_hints_component_creator.cc
[rename] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/optimization_guide/test_hints_component_creator.h
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_hints.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_hints.h
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_hints_unittest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_optimization_guide.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_optimization_guide.h
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/content/previews_optimization_guide_unittest.cc
[modify] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/core/BUILD.gn
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/core/previews_constants.cc
[add] https://crrev.com/e72337f49c13f7fd4e874ae0c1a38fb74ba69675/components/previews/core/previews_constants.h

Is this fixed now?
Status: Fixed (was: Started)
It is.

Sign in to add a comment