• Giovanni Ortuño Urquidi's avatar
    Revert "Eliminate unnecessary proto::Configuration copies" · e9de970c
    Giovanni Ortuño Urquidi authored
    This reverts commit 1b420fdb.
    
    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/1354745Reviewed-by: default avatarGiovanni Ortuño Urquidi <ortuno@chromium.org>
    Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#612044}
    e9de970c
previews_hints_unittest.cc 21.4 KB