Repository navigation
Conversation
github-actions
Bot
added
engine
e08aace to
2948267
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a GradientTextureCache to cache gradient ramp textures based on their colors and stops, reducing the overhead of recreating textures. The cache is integrated into ContentContext and managed per frame. The review feedback suggests optimizing the cache lookup by implementing heterogeneous lookup with absl::flat_hash_map and a GradientKeyView to avoid vector copies and heap allocations on cache hits.
| /// @brief The colors and stops that define a gradient ramp. | ||
| struct GradientKey { | ||
| std::vector<Color> colors; | ||
| std::vector<Scalar> stops; | ||
|
|
||
| bool operator==(const GradientKey&) const = default; | ||
|
|
||
| template <typename H> | ||
| friend H AbslHashValue(H h, const GradientKey& key) { | ||
| return H::combine(std::move(h), key.colors, key.stops); | ||
| } | ||
| }; | ||
|
|
||
| struct GradientTextureCacheData { | ||
| std::shared_ptr<Texture> texture; | ||
| bool used_this_frame = true; | ||
| }; | ||
|
|
||
| absl::flat_hash_map<GradientKey, GradientTextureCacheData> entries_; |
There was a problem hiding this comment.
To avoid copying the colors and stops vectors on every cache lookup (which causes heap allocations in a hot rendering path), we can use heterogeneous lookup with absl::flat_hash_map. By defining a GradientKeyView and transparent hash/equal structures, we can perform lookups using references to the existing vectors without copying them.
struct GradientKeyView {
const std::vector<Color>& colors;
const std::vector<Scalar>& stops;
template <typename H>
friend H AbslHashValue(H h, const GradientKeyView& key) {
return H::combine(std::move(h), key.colors, key.stops);
}
};
/// @brief The colors and stops that define a gradient ramp.
struct GradientKey {
std::vector<Color> colors;
std::vector<Scalar> stops;
bool operator==(const GradientKey&) const = default;
template <typename H>
friend H AbslHashValue(H h, const GradientKey& key) {
return H::combine(std::move(h), key.colors, key.stops);
}
};
struct GradientKeyHash {
using is_transparent = void;
size_t operator()(const GradientKey& key) const {
return absl::Hash<GradientKey>{}(key);
}
size_t operator()(const GradientKeyView& key) const {
return absl::Hash<GradientKeyView>{}(key);
}
};
struct GradientKeyEqual {
using is_transparent = void;
bool operator()(const GradientKey& lhs, const GradientKey& rhs) const {
return lhs.colors == rhs.colors && lhs.stops == rhs.stops;
}
bool operator()(const GradientKey& lhs, const GradientKeyView& rhs) const {
return lhs.colors == rhs.colors && lhs.stops == rhs.stops;
}
};
struct GradientTextureCacheData {
std::shared_ptr<Texture> texture;
bool used_this_frame = true;
};
absl::flat_hash_map<GradientKey,
GradientTextureCacheData,
GradientKeyHash,
GradientKeyEqual>
entries_;| std::shared_ptr<Texture> GradientTextureCache::Lookup( | ||
| const std::shared_ptr<Context>& context, | ||
| const std::vector<Color>& colors, | ||
| const std::vector<Scalar>& stops) { | ||
| GradientKey key{.colors = colors, .stops = stops}; | ||
| auto it = entries_.find(key); | ||
| if (it != entries_.end()) { | ||
| it->second.used_this_frame = true; | ||
| return it->second.texture; | ||
| } | ||
|
|
||
| GradientData gradient_data = CreateGradientBuffer(colors, stops); | ||
| std::shared_ptr<Texture> texture = | ||
| CreateGradientTexture(gradient_data, context); | ||
| if (!texture) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| entries_[std::move(key)] = | ||
| GradientTextureCacheData{.texture = texture, .used_this_frame = true}; | ||
| return texture; | ||
| } |
There was a problem hiding this comment.
Update the Lookup implementation to use GradientKeyView for the cache lookup, avoiding vector copies on cache hits. We only construct and copy into a GradientKey when inserting a new entry on a cache miss.
std::shared_ptr<Texture> GradientTextureCache::Lookup(
const std::shared_ptr<Context>& context,
const std::vector<Color>& colors,
const std::vector<Scalar>& stops) {
GradientKeyView key{.colors = colors, .stops = stops};
auto it = entries_.find(key);
if (it != entries_.end()) {
it->second.used_this_frame = true;
return it->second.texture;
}
GradientData gradient_data = CreateGradientBuffer(colors, stops);
std::shared_ptr<Texture> texture =
CreateGradientTexture(gradient_data, context);
if (!texture) {
return nullptr;
}
entries_[GradientKey{.colors = colors, .stops = stops}] =
GradientTextureCacheData{.texture = texture, .used_this_frame = true};
return texture;
}
|
After discussion with @gaaclarke, I am closing this out. For improving this performance, we will prefer other approaches rather than caching. |
Just in case it's helpful, i tested this in the morning on real hardware, while profiling same gradients issue and it increased performance a lot. AMD Ryzen AI 9 HX 370 with Radeon 890M. The app is a production desktop app with a full-window gradient/shader background, gradient fades on scrollables and network images. Engine: master e619960, profile build, the only change is this PR.
Per-frame blit passes from gradient texture uploads drop to about 0 in all scroll scenarios |
|
@krll-kov there wasn't a lot of explanation provided in the message that closed this out. I pushed against adding yet another cache because we can get the same results by calculating the gradients in the fragment shaders when we have SSBO available. That's a better solution since it has more consistent performance (one of the design goals of Impeller) and avoid's another vector for bugs/memory issues. SSBO hasn't been wired up for Windows yet though, so it will take some time to get around to it. We want SSBO wired up regardless because it can help in more cases. |
Functionality is based off of the existing
TextShadowCache.Tested this with the "Scrolling basic LinearGradient" benchmark app from #190401. I ran it 3 times. On both my Windows VM and my Mac:
Note that this benchmark draws one single gradient multiple times. This is the best case scenario for the gradient cache. With an app that draws different gradients (different colors and/or stops), the performance improvement will not be as drastic.
On Windows (VM with no GPU):
Before:
Full results
After:
Full results
On Mac (my local dev machine, M3 MacBook Pro):
Before:
Full results
After:
Full results
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.