Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824883006:
[ash-md] Reduce thread hopping for cheap wallpaper color extraction. (Closed)
Description[ash-md] Reduce thread hopping for cheap wallpaper color extraction.
Recent optimizations to the color_analysis::CalculateProminentColorOfBitmap()
has made it more expensive to perform color extraction asynchronously in some
cases. This change uses a simple image size heuristic to decide whether to
extract colors synchronously or asynchronously.
The optimizations made the 0-3 minute scale for the
'Ash.Wallpaper.TimeSpentExtractingColors' histogram overkill so it was replaced
with 'Ash.Wallpaper.ColorExtraction.Durations' one which uses a 0-10 second
scale.
This change also includes a new 'Ash.Wallpaper.ColorExtraction.UserDelay'
histogram that includes the thread switching delays incurred by asynchronous
calculations.
BUG=595010, 712714
TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest*
Review-Url: https://codereview.chromium.org/2824883006
Cr-Commit-Position: refs/heads/master@{#467075}
Committed: https://chromium.googlesource.com/chromium/src/+/e4135c8d12a6554d16e3bf16a8b79bb6462aa7d8
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressed patch set 1 comments. #Patch Set 3 : Removed 'BooleanSynchronous' from histograms.xml. #
Total comments: 10
Patch Set 4 : Addressed patch set 3 comments. #
Total comments: 8
Patch Set 5 : Merge branch 'master' into ash_wallpaper_timespentextractingcolors #Patch Set 6 : Addressed patch set 4 comments. #
Total comments: 7
Patch Set 7 : Addressed comments from patch set patch set 6. #
Messages
Total messages: 40 (25 generated)
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysisCalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. As a side effect some histograms have been deprecated and new ones added. BUG=712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* ========== to ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysisCalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. As a side effect some histograms have been deprecated and new ones added. BUG=595010, 712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* ==========
bruthig@chromium.org changed reviewers: + estade@chromium.org
Evan, can you take a first look before I send it to OWNERs?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nit: I don't think the histogram change is a side effect, it's a desired change regardless of the threading optimization and it would be good to expand on that in the CL description. https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... File components/wallpaper/wallpaper_color_calculator.cc (right): https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:41: bool ShouldCalculateAsync(const gfx::ImageSkia& image) { I think the name of this is backwards? https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:42: return kMaxPixelsForSynchronousCalculation >= image.width() * image.height(); nit: do you think this reads more clearly as width * height <= Max? https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:83: } else { nit: no else after return. https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2281: + The asynchronous time taken to extract colors from wallpapers. Recorded each the name and description of this histogram seem a little off. This should correlate to the user-visible delay before the wallpaper color is extracted, but much (in some cases) most of that time is not spent extracting colors. (Also "the asynchronous time taken" doesn't really make sense. I understand what this means only because I'm familiar with the code.) https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2287: +<histogram name="Ash.Wallpaper.ColorExtraction.ExecutionType" I don't think this is necessary. Can't we get this by comparing the summed buckets of TimeSpentExtractingColors and AsyncTimeSpent[...]
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysisCalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. As a side effect some histograms have been deprecated and new ones added. BUG=595010, 712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* ========== to ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysis::CalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously in some cases. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. The optimizations made the 0-3 minute scale for the 'Ash.Wallpaper.TimeSpentExtractingColors' histogram overkill so it was replaced with 'Ash.Wallpaper.ColorExtraction.Durations' one which uses a 0-10 second scale. This change also includes a new 'Ash.Wallpaper.ColorExtraction.UserDelay' histogram that includes the thread switching delays incurred by asynchronous calculations. BUG=595010, 712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* ==========
Evan, Can you take another look? On 2017/04/19 13:44:31, Evan Stade wrote: > nit: I don't think the histogram change is a side effect, it's a desired change > regardless of the threading optimization and it would be good to expand on that > in the CL description. Done. https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... File components/wallpaper/wallpaper_color_calculator.cc (right): https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:41: bool ShouldCalculateAsync(const gfx::ImageSkia& image) { On 2017/04/19 13:44:31, Evan Stade wrote: > I think the name of this is backwards? Nice catch, thx! https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:42: return kMaxPixelsForSynchronousCalculation >= image.width() * image.height(); On 2017/04/19 13:44:31, Evan Stade wrote: > nit: do you think this reads more clearly as width * height <= Max? Done. https://codereview.chromium.org/2824883006/diff/1/components/wallpaper/wallpa... components/wallpaper/wallpaper_color_calculator.cc:83: } else { On 2017/04/19 13:44:31, Evan Stade wrote: > nit: no else after return. Done. https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2281: + The asynchronous time taken to extract colors from wallpapers. Recorded each On 2017/04/19 13:44:31, Evan Stade wrote: > the name and description of this histogram seem a little off. This should > correlate to the user-visible delay before the wallpaper color is extracted, but > much (in some cases) most of that time is not spent extracting colors. > > (Also "the asynchronous time taken" doesn't really make sense. I understand what > this means only because I'm familiar with the code.) I've reworked the naming and descriptions. WDYT? https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2287: +<histogram name="Ash.Wallpaper.ColorExtraction.ExecutionType" On 2017/04/19 13:44:31, Evan Stade wrote: > I don't think this is necessary. Can't we get this by comparing the summed > buckets of TimeSpentExtractingColors and AsyncTimeSpent[...] Done.
https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator.cc (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.cc:21: // color. nit: perhaps explain the motivation briefly, e.g. "extraction on images smaller than this should run so quickly there's no need to offload the work to a different thread." https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.cc:74: WallpaperColorCalculator::NotifyCalculationComplete(prominent_color); nit: WallpaperColorCalculator:: isn't necessary is it? https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator.h (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.h:41: // initiated. Observers may be notified synchronously or asynchronously. nit: each file should be consistent about one or two spaces after period (I prefer one but I think the rule is just not to clash within each file) https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:125: AsynchronousMetricsForFailedExtraction) { is this actually failed extraction or failed task posting? it seems like failed extraction would be more interesting to test. https://codereview.chromium.org/2824883006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824883006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2343: + wallpaper image changes. NOTE, this measure also included the timespent nit: time spent
https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824883006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2281: + The asynchronous time taken to extract colors from wallpapers. Recorded each On 2017/04/19 15:15:52, bruthig wrote: > On 2017/04/19 13:44:31, Evan Stade wrote: > > the name and description of this histogram seem a little off. This should > > correlate to the user-visible delay before the wallpaper color is extracted, > but > > much (in some cases) most of that time is not spent extracting colors. > > > > (Also "the asynchronous time taken" doesn't really make sense. I understand > what > > this means only because I'm familiar with the code.) > > I've reworked the naming and descriptions. > WDYT? looks good, thanks
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bruthig@chromium.org changed reviewers: + bshe@chromium.org, mpearson@chromium.org
bshe@, can you PTAL? mpearson@, can you take a look at the histogram usage in: - wallpaper_color_calculator.cc - wallpaper_color_calculator_unittest.cc - histograms.xml https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator.cc (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.cc:21: // color. On 2017/04/20 21:37:15, Evan Stade wrote: > nit: perhaps explain the motivation briefly, e.g. "extraction on images smaller > than this should run so quickly there's no need to offload the work to a > different thread." Done. https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.cc:74: WallpaperColorCalculator::NotifyCalculationComplete(prominent_color); On 2017/04/20 21:37:15, Evan Stade wrote: > nit: WallpaperColorCalculator:: isn't necessary is it? Whoops, removed. https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator.h (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator.h:41: // initiated. Observers may be notified synchronously or asynchronously. On 2017/04/20 21:37:15, Evan Stade wrote: > nit: each file should be consistent about one or two spaces after period (I > prefer one but I think the rule is just not to clash within each file) Done. https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/40001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:125: AsynchronousMetricsForFailedExtraction) { On 2017/04/20 21:37:15, Evan Stade wrote: > is this actually failed extraction or failed task posting? > > it seems like failed extraction would be more interesting to test. Renamed and added test for failed extraction. https://codereview.chromium.org/2824883006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824883006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2343: + wallpaper image changes. NOTE, this measure also included the timespent On 2017/04/20 21:37:15, Evan Stade wrote: > nit: time spent Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:67: canvas.FillRect(gfx::Rect(size), kGray); nitty nit nit: canvas.DrawColor(kGray); https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:143: EXPECT_FALSE(calculator_->StartCalculation()); nit: wouldn't this test pass even without the NullTaskRunner since you haven't called RunUntilIdle()? https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:194: class WallPaperColorCalculatorSyncTest : public testing::Test { do you really need a whole other test class for this? Just exposing calculator/image initialization to test classes would allow for reusing more code. The problem I see with multiple test classes is that it's not very scalable for the future when you want to change other initialization parameters. (even just calling set_image_for_test() should be sufficient, no?) https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:243: CreateNonColorProducingImage(gfx::Size(5, 5))); I would prefer not having _for_test() functions if avoidable. For example in this case, can't we just create a new WallpaperColorCalculator when we want a different image?
histograms.xml lgtm
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estade@, take another look if you wish, but not required. bshe@, can you PTAL? https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:67: canvas.FillRect(gfx::Rect(size), kGray); On 2017/04/21 16:57:10, Evan Stade wrote: > nitty nit nit: canvas.DrawColor(kGray); Done. https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:143: EXPECT_FALSE(calculator_->StartCalculation()); On 2017/04/21 16:57:10, Evan Stade wrote: > nit: wouldn't this test pass even without the NullTaskRunner since you haven't > called RunUntilIdle()? The test wouldn't pass because this EXPECT (line 143) would fail, however you're right, the histogram expectations below would 'incorrectly' pass. I've added a RunUntilIdle() to be safe. https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:194: class WallPaperColorCalculatorSyncTest : public testing::Test { On 2017/04/21 16:57:10, Evan Stade wrote: > do you really need a whole other test class for this? Just exposing > calculator/image initialization to test classes would allow for reusing more > code. The problem I see with multiple test classes is that it's not very > scalable for the future when you want to change other initialization parameters. > > (even just calling set_image_for_test() should be sufficient, no?) Collapsed into a single test fixture, but used typedefs to group tests. WDYT? https://codereview.chromium.org/2824883006/diff/60001/components/wallpaper/wa... components/wallpaper/wallpaper_color_calculator_unittest.cc:243: CreateNonColorProducingImage(gfx::Size(5, 5))); On 2017/04/21 16:57:10, Evan Stade wrote: > I would prefer not having _for_test() functions if avoidable. For example in > this case, can't we just create a new WallpaperColorCalculator when we want a > different image? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... File components/wallpaper/wallpaper_color_calculator.cc (right): https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator.cc:29: // NOTE: |image| is intentionally a copy to ensure it exists for the duration of thumbs up https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:114: InstallTaskRunner(task_runner_); why do you call this here? Doesn't it need to be called after CreateCalculator, which hasn't been called yet? (I would actually make CreateCalculator with async size part of the ctor, and then just make sure if a test calls CreateCalculator again it re-makes it without any issue.) https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:174: StartCalculationReturnsFalseWhenPostingTaskFails) { nit: you're already testing this with the above test. This one is redundant. https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:246: CreateCalculator(CreateNonColorProducingImage(gfx::Size(5, 5))); nit: use the constant?
On 2017/04/25 16:24:39, Evan Stade wrote: > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > File components/wallpaper/wallpaper_color_calculator.cc (right): > > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > components/wallpaper/wallpaper_color_calculator.cc:29: // NOTE: |image| is > intentionally a copy to ensure it exists for the duration of > thumbs up > > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): > > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > components/wallpaper/wallpaper_color_calculator_unittest.cc:114: > InstallTaskRunner(task_runner_); > why do you call this here? Doesn't it need to be called after CreateCalculator, > which hasn't been called yet? > > (I would actually make CreateCalculator with async size part of the ctor, and > then just make sure if a test calls CreateCalculator again it re-makes it > without any issue.) > > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > components/wallpaper/wallpaper_color_calculator_unittest.cc:174: > StartCalculationReturnsFalseWhenPostingTaskFails) { > nit: you're already testing this with the above test. This one is redundant. > > https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... > components/wallpaper/wallpaper_color_calculator_unittest.cc:246: > CreateCalculator(CreateNonColorProducingImage(gfx::Size(5, 5))); > nit: use the constant? sorry for delay. lgtm with nits addressed.
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sending to CQ... estade@, take a look if you wish but I believe I did as you requested. https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... File components/wallpaper/wallpaper_color_calculator_unittest.cc (right): https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:114: InstallTaskRunner(task_runner_); On 2017/04/25 16:24:39, Evan Stade wrote: > why do you call this here? Doesn't it need to be called after CreateCalculator, > which hasn't been called yet? > The order of CreateCalculator() and InstallTaskRunner() shouldn't matter. > (I would actually make CreateCalculator with async size part of the ctor, and > then just make sure if a test calls CreateCalculator again it re-makes it > without any issue.) CreateCalculator() already supports this. FTR I made the ctor construct a calculator with an async, color yielding image. https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:174: StartCalculationReturnsFalseWhenPostingTaskFails) { On 2017/04/25 16:24:39, Evan Stade wrote: > nit: you're already testing this with the above test. This one is redundant. Removed. https://codereview.chromium.org/2824883006/diff/100001/components/wallpaper/w... components/wallpaper/wallpaper_color_calculator_unittest.cc:246: CreateCalculator(CreateNonColorProducingImage(gfx::Size(5, 5))); On 2017/04/25 16:24:39, Evan Stade wrote: > nit: use the constant? Whoops, done.
The CQ bit was unchecked by bruthig@chromium.org
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2824883006/#ps120001 (title: "Addressed comments from patch set patch set 6.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493144209702140,
"parent_rev": "2a657ecbdd2743e0cc7c4e5130a5b1caac88ec9e", "commit_rev":
"e4135c8d12a6554d16e3bf16a8b79bb6462aa7d8"}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysis::CalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously in some cases. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. The optimizations made the 0-3 minute scale for the 'Ash.Wallpaper.TimeSpentExtractingColors' histogram overkill so it was replaced with 'Ash.Wallpaper.ColorExtraction.Durations' one which uses a 0-10 second scale. This change also includes a new 'Ash.Wallpaper.ColorExtraction.UserDelay' histogram that includes the thread switching delays incurred by asynchronous calculations. BUG=595010, 712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* ========== to ========== [ash-md] Reduce thread hopping for cheap wallpaper color extraction. Recent optimizations to the color_analysis::CalculateProminentColorOfBitmap() has made it more expensive to perform color extraction asynchronously in some cases. This change uses a simple image size heuristic to decide whether to extract colors synchronously or asynchronously. The optimizations made the 0-3 minute scale for the 'Ash.Wallpaper.TimeSpentExtractingColors' histogram overkill so it was replaced with 'Ash.Wallpaper.ColorExtraction.Durations' one which uses a 0-10 second scale. This change also includes a new 'Ash.Wallpaper.ColorExtraction.UserDelay' histogram that includes the thread switching delays incurred by asynchronous calculations. BUG=595010, 712714 TEST=components_unittests --gtest_filter=*WallPaperColorCalculator*yncTest* Review-Url: https://codereview.chromium.org/2824883006 Cr-Commit-Position: refs/heads/master@{#467075} Committed: https://chromium.googlesource.com/chromium/src/+/e4135c8d12a6554d16e3bf16a8b7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e4135c8d12a6554d16e3bf16a8b7...
Message was sent while issue was closed.
On 2017/04/25 18:16:41, bruthig wrote: > stuff all good, thanks for updates |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
