perf(ui): hoist anonymous popup builders to named methods to avoid inline allocations#8735
Open
danteboe wants to merge 6 commits into
Open
perf(ui): hoist anonymous popup builders to named methods to avoid inline allocations#8735danteboe wants to merge 6 commits into
danteboe wants to merge 6 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>
|
|
Contributor
Reviewer's GuideRefactors several hot-path UI and logging code paths to reduce heap allocations by hoisting inline closures/builders into reusable methods, introducing reuseable buffers, and tightening serialization, while also allowing perf commit type in commitlint. Sequence diagram for the renamed popup builder flow in ViewTitlesequenceDiagram
actor User
participant FlowyButton
participant PopoverController
participant _ViewTitleState
participant ViewTitleBloc
participant RenameViewPopover
User->>FlowyButton: onPressed
FlowyButton->>PopoverController: popupBuilder(context)
PopoverController->>_ViewTitleState: _buildRenamePopover(context)
_ViewTitleState->>_ViewTitleState: _latestViewTitleState = state
alt _latestViewTitleState is set
_ViewTitleState->>_ViewTitleState: _resetTextEditingController(_latestViewTitleState)
_ViewTitleState->>RenameViewPopover: RenameViewPopover(view, name, popoverController, icon, _latestViewTitleState.icon, tabs)
else _latestViewTitleState is null
_ViewTitleState->>ViewTitleBloc: context.read<ViewTitleBloc>()
ViewTitleBloc-->>_ViewTitleState: state
_ViewTitleState->>_ViewTitleState: _resetTextEditingController(state)
_ViewTitleState->>RenameViewPopover: RenameViewPopover(view, name, popoverController, icon, state.icon, tabs)
end
Flow diagram for thread_local log buffer reuse in FlowyFormattingLayerflowchart LR
subgraph ThreadLocal
LOG_BUFFER
end
subgraph FlowyFormattingLayer
format_span_as_json
on_event
make_writer
end
format_span_as_json --> LOG_BUFFER
on_event --> LOG_BUFFER
LOG_BUFFER --> format_span_as_json
LOG_BUFFER --> on_event
format_span_as_json -->|serde_json::Serializer| make_writer
on_event -->|serde_json::Serializer| make_writer
make_writer -->|"make_writer().write_all"| OutputStream
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:
- Caching
_cachedBreadcrumbsas concreteWidgets inViewTitleBarcan easily go stale when inherited widgets (theme, locale, text scale, etc.) change without the cache key changing; consider caching only the input data and rebuilding the widgets each build or making the widget fully derived from bloc state again. - In
FlowyFormattingLayer,LOG_BUFFER+span_to_jsonnow write directly to the writer whileemitstill takes aVec<u8>; it would be clearer to either removeemitor refactor so there is a single consistent code path for formatting and emitting to avoid confusion about ownership and buffer lifetimes. - This PR deletes several files (
assets/translations/mr-IN.json,.cargo/config.toml,flowy-sqlite/.env, macOS/Xcode build artifacts); please double-check that these removals are intentional and not unrelated to the performance-focused changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Caching `_cachedBreadcrumbs` as concrete `Widget`s in `ViewTitleBar` can easily go stale when inherited widgets (theme, locale, text scale, etc.) change without the cache key changing; consider caching only the input data and rebuilding the widgets each build or making the widget fully derived from bloc state again.
- In `FlowyFormattingLayer`, `LOG_BUFFER` + `span_to_json` now write directly to the writer while `emit` still takes a `Vec<u8>`; it would be clearer to either remove `emit` or refactor so there is a single consistent code path for formatting and emitting to avoid confusion about ownership and buffer lifetimes.
- This PR deletes several files (`assets/translations/mr-IN.json`, `.cargo/config.toml`, `flowy-sqlite/.env`, macOS/Xcode build artifacts); please double-check that these removals are intentional and not unrelated to the performance-focused changes.
## Individual Comments
### Comment 1
<location path="frontend/appflowy_flutter/lib/workspace/presentation/widgets/view_title_bar.dart" line_range="103-112" />
<code_context>
+ final currentKey = '${ancestors.map((a) => a.id).join(',')}|${state.isDeleted}|${pageAccessLevelState.isEditable}|${pageAccessLevelState.sectionType.name}';
</code_context>
<issue_to_address>
**issue (bug_risk):** The breadcrumb cache key ignores ancestor display properties, so renaming an ancestor may not update the breadcrumb labels.
Because the key only uses ancestor IDs plus deletion and access flags, any breadcrumb data derived from mutable ancestor fields (e.g., names, icons) won’t invalidate the cache. Renaming an ancestor will still reuse `_cachedBreadcrumbs`, leaving stale labels until some other state change occurs. Consider including the relevant mutable fields in the key, or refactoring to a more targeted memoization (e.g., per‑ancestor caching keyed by id + version).
</issue_to_address>
### Comment 2
<location path="frontend/rust-lib/flowy-ai-pub/src/persistence/chat_sql.rs" line_range="32-40" />
<code_context>
pub fn new(chat_id: String, metadata: Value, rag_ids: Vec<Uuid>, is_sync: bool) -> Self {
- let rag_ids = rag_ids.iter().map(|v| v.to_string()).collect::<Vec<_>>();
+ // Serialize rag ids without allocating an intermediate Vec<String>.
+ fn serialize_rag_ids_from_uuids(rag_ids: &[Uuid]) -> String {
+ let mut buf = Vec::new();
+ let mut serializer = serde_json::Serializer::new(&mut buf);
+ let mut seq = serializer.serialize_seq(Some(rag_ids.len())).unwrap();
+ for id in rag_ids.iter() {
+ seq.serialize_element(&id.to_string()).unwrap();
+ }
+ seq.end().unwrap();
+ String::from_utf8(buf).unwrap_or_default()
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `unwrap_or_default()` on `String::from_utf8` silently discards invalid data instead of surfacing an error.
Here, `String::from_utf8(buf).unwrap_or_default()` will return an empty string on invalid UTF‑8, hiding a real error in the JSON serialization path. Since we expect valid UTF‑8 for JSON UUIDs, this should fail fast instead (e.g. use `expect` with a clear message or `unwrap()`) rather than silently producing an empty `rag_ids` payload.
</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
+112
| final currentKey = '${ancestors.map((a) => a.id).join(',')}|${state.isDeleted}|${pageAccessLevelState.isEditable}|${pageAccessLevelState.sectionType.name}'; | ||
|
|
||
| if (_cachedKey != currentKey || _cachedBreadcrumbs.isEmpty) { | ||
| _cachedBreadcrumbs = _buildViewTitles( | ||
| context, | ||
| ancestors, | ||
| state.isDeleted, | ||
| pageAccessLevelState.isEditable, | ||
| pageAccessLevelState, | ||
| ); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The breadcrumb cache key ignores ancestor display properties, so renaming an ancestor may not update the breadcrumb labels.
Because the key only uses ancestor IDs plus deletion and access flags, any breadcrumb data derived from mutable ancestor fields (e.g., names, icons) won’t invalidate the cache. Renaming an ancestor will still reuse _cachedBreadcrumbs, leaving stale labels until some other state change occurs. Consider including the relevant mutable fields in the key, or refactoring to a more targeted memoization (e.g., per‑ancestor caching keyed by id + version).
Comment on lines
+32
to
+40
| fn serialize_rag_ids_from_uuids(rag_ids: &[Uuid]) -> String { | ||
| let mut buf = Vec::new(); | ||
| let mut serializer = serde_json::Serializer::new(&mut buf); | ||
| let mut seq = serializer.serialize_seq(Some(rag_ids.len())).unwrap(); | ||
| for id in rag_ids.iter() { | ||
| seq.serialize_element(&id.to_string()).unwrap(); | ||
| } | ||
| seq.end().unwrap(); | ||
| String::from_utf8(buf).unwrap_or_default() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using unwrap_or_default() on String::from_utf8 silently discards invalid data instead of surfacing an error.
Here, String::from_utf8(buf).unwrap_or_default() will return an empty string on invalid UTF‑8, hiding a real error in the JSON serialization path. Since we expect valid UTF‑8 for JSON UUIDs, this should fail fast instead (e.g. use expect with a clear message or unwrap()) rather than silently producing an empty rag_ids payload.
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
Performance impact
Testing
Risk and compatibility
Checklist
Summary by Sourcery
Optimize UI and backend hot paths by hoisting repeated allocation patterns into reusable state and buffers, reducing runtime allocations and improving performance.
Enhancements:
Chores:
perfcommit type in commitlint configuration for performance-focused changes.