Cherry pick PR #8050 to main: Reland "feat(cc): Disable resource pool reuse to reduce memory #7853"#11520
Cherry pick PR #8050 to main: Reland "feat(cc): Disable resource pool reuse to reduce memory #7853"#11520niranjanyardi wants to merge 2 commits into
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
…e#7853" (youtube#8050) This relands PR# 7853, which was reverted due to breaking `PartialRasterTileManagerTest.PartialRasterSuccessfullyEnabled` and `PartialRasterTileManagerTest.PartialTileImageDecode` on Cobalt. This reland re-introduces the test refactoring but disables the failing tests specifically for Cobalt builds using `#if BUILDFLAG(IS_COBALT) GTEST_SKIP()`. This allows the valuable test improvements to be landed without affecting the Cobalt build. Original PR description--------------------------------------- By marking resources with 'avoid_reuse', they are freed immediately after use instead of being kept in the resource pool. This ensures tiles are rastered on-demand. This change significantly reduces the tile_memory usage, ~20M when navigating fast through videos, with no obvious negative performance impact. Original PR: youtube#7853 Bug: 424591313 (cherry picked from commit 8ec6d83)
ce2ef00 to
13ba2c4
Compare
There was a problem hiding this comment.
Code Review
This pull request disables several unit tests in resource_pool_unittest.cc, tile_manager_unittest.cc, and layer_tree_host_impl_unittest.cc when building for Cobalt by using GTEST_SKIP(). The reviewer recommended simplifying the preprocessor directives by only wrapping the GTEST_SKIP() call within the #if block, rather than enclosing the entire remaining test body in an #else block, which reduces unnecessary boilerplate and aligns with clean coding standards.
| #if BUILDFLAG(IS_COBALT) | ||
| GTEST_SKIP() << "Cobalt doesn't support resource reuse."; | ||
| #else |
There was a problem hiding this comment.
Since GTEST_SKIP() internally behaves like a return statement, any code following it in the same test function will not be executed. Therefore, wrapping the entire test body in #else and #endif is unnecessary and adds preprocessor boilerplate.
You can simplify this pattern across all modified tests in this PR (including those in cc/resources/resource_pool_unittest.cc, cc/tiles/tile_manager_unittest.cc, and cc/trees/layer_tree_host_impl_unittest.cc) by only wrapping the GTEST_SKIP() call in the #if block, leaving the rest of the test body unwrapped.
For example:
TEST_F(ResourcePoolTest, EventuallyEvictAndFlush) {
#if BUILDFLAG(IS_COBALT)
GTEST_SKIP() << "Cobalt doesn't support resource reuse.";
#endif
gfx::Size size(100, 100);
viz::SharedImageFormat format = viz::SinglePlaneFormat::kRGBA_8888;
gfx::ColorSpace color_space = gfx::ColorSpace::CreateSRGB();
...
}This significantly improves readability and maintainability by avoiding nested preprocessor blocks and reducing the diff size.
References
- Adhere to upstream Chromium style guides which favor clean, readable code with minimal preprocessor noise. Avoid wrapping entire function bodies in preprocessor conditionals when early returns or macros like GTEST_SKIP() can be used directly. (link)
This cherry-picks PR #8050 (and related test updates) to main, resolving merge conflicts in
cc/resources/resource_pool.cc.Original PR #8050 Description
This relands PR# 7853, which was reverted due to breaking
PartialRasterTileManagerTest.PartialRasterSuccessfullyEnabledandPartialRasterTileManagerTest.PartialTileImageDecodeon Cobalt.This reland re-introduces the test refactoring but disables the failing tests specifically for Cobalt builds using
#if BUILDFLAG(IS_COBALT) GTEST_SKIP(). This allows the valuable test improvements to be landed without affecting the Cobalt build.Original PR description---------------------------------------
By marking resources with 'avoid_reuse', they are freed immediately after use instead of being kept in the resource pool. This ensures tiles are rastered on-demand.
This change significantly reduces the tile_memory usage, ~20M when navigating fast through videos, with no obvious negative performance impact.
Original PR: #7853
Bug: b/424591313