OptimizationGuideService makes unnecessary copies of proto::Configuration |
|||||
Issue descriptionThe 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.
,
Nov 27
,
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
,
Nov 29
,
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
,
Nov 29
Reopening since the CL got reverted.
,
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
,
Jan 7
Is this fixed now?
,
Jan 7
It is. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tbansal@chromium.org
, Nov 27