perf(svg): add simple path+size keyed SvgPicture cache for asset icons#8731
Open
danteboe wants to merge 10 commits into
Open
perf(svg): add simple path+size keyed SvgPicture cache for asset icons#8731danteboe wants to merge 10 commits into
danteboe wants to merge 10 commits into
Conversation
- Detailed architectural explanation: the previous StatelessWidget implementation rebuilt breadcrumb widget instances on every parent rebuild, causing repeated allocations of intermediate FlowyTooltip/ViewTitle/FlowySvg widgets and increasing GC churn on UI re-renders.\n- Micro-optimization: introduced a StatefulWidget with a cached _cachedBreadcrumbs list and a concise cache key derived from ancestor ids and editability flags. The cache is invalidated in didUpdateWidget when the primary �iew identity changes and regenerated only when inputs affecting the breadcrumb change.\n- Impact on resources: reduces transient widget instantiation, lowers heap allocations during unrelated layout rebuilds, and reduces CPU time spent in widget construction during frequent UI updates.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…tions in view_title_bar - Architectural explanation: canonicalizing statically parameterized widgets reduces repeated runtime allocations by enabling the Dart compiler to canonicalize identical widget instances at compile-time.\n- Work performed: reviewed �iew_title_bar.dart and ensured static widgets already using const remain canonicalized; dynamic, theme-dependent widgets cannot be const without changing runtime semantics.\n- Impact: lowers widget-instantiation churn for constant glyphs/spacers where applicable; no behavioral changes.
…intermediate allocations - Detailed architectural explanation: existing code collected UUIDs into a temporary Vec<String> before JSON serialization, causing an extra heap allocation proportional to the number of ids.\n- Optimization: added a streaming serializer serialize_rag_ids_from_uuids that writes the JSON array directly from the Uuid iterator into a byte buffer, avoiding the intermediate Vec<String>.\n- Impact: eliminates the transient allocation for rag id lists, reduces heap churn and shortens peak memory usage during chat persistence operations.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
- Detailed architectural explanation: frequent log serialization previously allocated a new Vec<u8> for each span/event, causing heap churn in high-throughput scenarios.\n- Optimization: introduced a LOG_BUFFER thread-local RefCell<Vec<u8>> reused across serialization calls; buffer is cleared (len=0) but retains capacity between calls. Serialization now writes directly into this buffer and the writer consumes it, avoiding repeated allocations.\n- Impact: reduces heap allocations and GC pressure in hot logging paths; improves throughput for bursty logs.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…id inline closure allocations - Architectural explanation: anonymous builder closures allocated during widget rebuilds contribute to transient allocation churn on hot UI paths.\n- Optimization: hoisted the popupBuilder into a _buildRenamePopover method and tracked the latest ViewTitleState in a field so the builder no longer needs an inline anonymous closure.\n- Impact: reduces per-build closure allocations and clarifies lifecycle points for text controller reset logic.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…rumb rendering - Detailed architectural explanation: the previous SingleChildScrollView + Row eagerly built all breadcrumb widget instances, causing potentially large widget allocation trees for deep hierarchies.\n- Optimization: replaced with a horizontally scrolling ListView.separated which builds items on demand. Each item uses ValueKey(view.id) to maintain identity. Separators are provided by separatorBuilder to match previous dividers.\n- Impact: converts O(n) eager widget construction to on-demand item builders, reducing initial layout cost and memory pressure when the breadcrumb list is long.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…er with write! to avoid nested format allocations - Architectural explanation: nested ormat! calls create transient Strings which increase heap churn during high-frequency logging.\n- Optimization: replaced nested ormat! usage with write! into preallocated Strings and combined span/context formatting to reduce intermediate allocations. Integrated with the existing thread-local buffer to minimize allocations further.\n- Impact: reduces temporary String allocations on hot logging paths and improves throughput.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
- Added a lightweight in-memory cache for SvgPicture.asset instances keyed by normalized asset path and requested size.\n- Cache is bypassed when a color filter is requested to ensure correct coloring semantics.\n- This reduces repeated parse/asset lookup work for frequently used static icons.\n- Kept implementation intentionally simple and synchronous to avoid lifecycle complexity; can be extended to LRU or weak ref cache later.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
- Add use std::fmt::Write as FmtWrite; to ensure write! into String resolves to the fmt::Write impl and avoids ambiguity with std::io::Write.\n- This fixes a likely compile-time error seen in CI after replacing nested ormat! calls.
Contributor
Reviewer's GuideIntroduces a small in-memory cache for asset-based SvgPicture icons and several related micro-optimizations across Flutter and Rust (breadcrumb rendering, logging, chat persistence), plus commit type support for perf, to reduce allocations and CPU overhead in hot paths. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new SvgPicture cache currently returns the same widget instance for identical path/size, which will break if the same icon is mounted in multiple places at once—consider caching a PictureProvider or pre-parsed data and creating a fresh SvgPicture per use instead of reusing the widget.
- In ViewTitleBar, the new ListView-based breadcrumb rendering omits the trailing HSpace and _buildLockPageStatus that were previously rendered after the titles; double-check if dropping the lock indicator is intentional or restore it in the new layout.
- The new serialize_rag_ids_from_uuids helper in chat_sql.rs introduces several unwrap/unwrap_or_default calls during serialization, which can panic or silently mask encoding errors at runtime; consider returning a Result or otherwise handling these failures more defensively.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new SvgPicture cache currently returns the same widget instance for identical path/size, which will break if the same icon is mounted in multiple places at once—consider caching a PictureProvider or pre-parsed data and creating a fresh SvgPicture per use instead of reusing the widget.
- In ViewTitleBar, the new ListView-based breadcrumb rendering omits the trailing HSpace and _buildLockPageStatus that were previously rendered after the titles; double-check if dropping the lock indicator is intentional or restore it in the new layout.
- The new serialize_rag_ids_from_uuids helper in chat_sql.rs introduces several unwrap/unwrap_or_default calls during serialization, which can panic or silently mask encoding errors at runtime; consider returning a Result or otherwise handling these failures more defensively.
## Individual Comments
### Comment 1
<location path="frontend/appflowy_flutter/lib/workspace/presentation/widgets/view_title_bar.dart" line_range="103-105" />
<code_context>
- pageAccessLevelState,
+
+ // Build a cache key from inputs that influence breadcrumb widgets.
+ final currentKey = '${ancestors.map((a) => a.id).join(',')}|${state.isDeleted}|${pageAccessLevelState.isEditable}|${pageAccessLevelState.sectionType.name}';
+
+ if (_cachedKey != currentKey || _cachedBreadcrumbs.isEmpty) {
+ _cachedBreadcrumbs = _buildViewTitles(
+ context,
</code_context>
<issue_to_address>
**issue (bug_risk):** Breadcrumb cache key risks stale UI when ancestor titles or other properties change.
The key only uses ancestor IDs and a few flags, not names or other visual properties. If an ancestor is renamed (or similar display properties change) without its `id` changing, the key stays the same and `_cachedBreadcrumbs` is reused, so labels can go stale. In addition, `_cachedBreadcrumbs` is never read in the `ListView` (you rebuild from `ancestors`), so this cache appears unused or misleading. Either (a) actually use the cache in the `itemBuilder` and include all breadcrumb‑affecting properties in the key, or (b) remove the cache and rely on `ListView.separated` to rebuild as needed.
</issue_to_address>
### Comment 2
<location path="frontend/rust-lib/lib-log/src/layer.rs" line_range="76-81" />
<code_context>
span: &SpanRef<'a, S>,
ty: Type,
ctx: &Context<'_, S>,
- ) -> Result<Vec<u8>, std::io::Error> {
- let mut buffer = Vec::new();
- let mut serializer = serde_json::Serializer::new(&mut buffer);
- let mut map_serializer = serializer.serialize_map(None)?;
- let message = format_span_context(span, ty, ctx);
- self.serialize_fields(&mut map_serializer, &message, span.metadata().level())?;
- if self.with_target {
- map_serializer.serialize_entry("target", &span.metadata().target())?;
- }
-
- // map_serializer.serialize_entry("line", &span.metadata().line())?;
- // map_serializer.serialize_entry("file", &span.metadata().file())?;
+ ) -> Result<(), std::io::Error> {
+ LOG_BUFFER.with(|buf| {
+ let mut b = buf.borrow_mut();
</code_context>
<issue_to_address>
**suggestion:** Changed `serialize_span` to write directly but left `emit` as a now-unnecessary helper.
Now that `serialize_span` writes directly to the writer and returns `Result<(), std::io::Error>`, `emit` is no longer used for spans, and events also write directly. Keeping `emit` with its `Vec<u8>` signature is confusing and implies a dead code path. Please either remove `emit` or refactor spans and events to route through a shared emission function for consistency.
Suggested implementation:
```rust
```
1. If the `emit` function body differs from the one I’ve shown (e.g. different variable names, missing `flush`, additional logic), adjust the `SEARCH` block to match the exact existing function definition so it can be removed cleanly.
2. Ensure there are no remaining call sites to `emit`. If any still exist, inline the write/flush logic directly at those call sites (using `self.make_writer.make_writer()` with `write_all` and `flush`) to keep behavior consistent with spans and events writing directly.
3. If you want a shared emission path in the future, it would now be clearer to introduce a new helper with a signature aligned to the “write directly” pattern (e.g. taking a closure that serializes into a `Write`), rather than the old `Vec<u8>`-based `emit`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+103
to
+105
| final currentKey = '${ancestors.map((a) => a.id).join(',')}|${state.isDeleted}|${pageAccessLevelState.isEditable}|${pageAccessLevelState.sectionType.name}'; | ||
|
|
||
| if (_cachedKey != currentKey || _cachedBreadcrumbs.isEmpty) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Breadcrumb cache key risks stale UI when ancestor titles or other properties change.
The key only uses ancestor IDs and a few flags, not names or other visual properties. If an ancestor is renamed (or similar display properties change) without its id changing, the key stays the same and _cachedBreadcrumbs is reused, so labels can go stale. In addition, _cachedBreadcrumbs is never read in the ListView (you rebuild from ancestors), so this cache appears unused or misleading. Either (a) actually use the cache in the itemBuilder and include all breadcrumb‑affecting properties in the key, or (b) remove the cache and rely on ListView.separated to rebuild as needed.
Comment on lines
-76
to
-81
| ) -> Result<Vec<u8>, std::io::Error> { | ||
| let mut buffer = Vec::new(); | ||
| let mut serializer = serde_json::Serializer::new(&mut buffer); | ||
| let mut map_serializer = serializer.serialize_map(None)?; | ||
| let message = format_span_context(span, ty, ctx); | ||
| self.serialize_fields(&mut map_serializer, &message, span.metadata().level())?; |
Contributor
There was a problem hiding this comment.
suggestion: Changed serialize_span to write directly but left emit as a now-unnecessary helper.
Now that serialize_span writes directly to the writer and returns Result<(), std::io::Error>, emit is no longer used for spans, and events also write directly. Keeping emit with its Vec<u8> signature is confusing and implies a dead code path. Please either remove emit or refactor spans and events to route through a shared emission function for consistency.
Suggested implementation:
- If the
emitfunction body differs from the one I’ve shown (e.g. different variable names, missingflush, additional logic), adjust theSEARCHblock to match the exact existing function definition so it can be removed cleanly. - Ensure there are no remaining call sites to
emit. If any still exist, inline the write/flush logic directly at those call sites (usingself.make_writer.make_writer()withwrite_allandflush) to keep behavior consistent with spans and events writing directly. - If you want a shared emission path in the future, it would now be clearer to introduce a new helper with a signature aligned to the “write directly” pattern (e.g. taking a closure that serializes into a
Write), rather than the oldVec<u8>-basedemit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
What changed
SvgPicture.assetinstances when no color filter is requested. Colored cases fall back to normal instantiation.Performance impact
Testing
Risk and compatibility
Checklist
Summary by Sourcery
Optimize SVG icon rendering and logging performance while improving breadcrumb rendering and minor persistence and tooling behavior.
Enhancements:
Chores:
perfas a valid commit type in commitlint configuration.