perf(logging): write formatted span/event messages into buffer with write! to avoid nested allocations#8729
perf(logging): write formatted span/event messages into buffer with write! to avoid nested allocations#8729danteboe wants to merge 9 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>
- 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.
Add 'perf' to the allowed ype-enum values so performance branches using perf(...) commit headers pass the repository commitlint check.
Reviewer's GuideOptimizes Rust logging and related code paths to reduce transient allocations by reusing buffers and using write!-based formatting, while also adding Flutter UI caching for breadcrumbs, improving ViewTitle popover state handling, simplifying RAG ID serialization, and allowing perf commit types. Sequence diagram for Rust logging buffer reuse in FlowyFormattingLayersequenceDiagram
participant TracingSubscriber
participant FlowyFormattingLayer
participant LOG_BUFFER
participant MakeWriter
participant Writer
TracingSubscriber->>FlowyFormattingLayer: on_event(event, ctx)
FlowyFormattingLayer->>LOG_BUFFER: borrow_mut()
LOG_BUFFER-->>FlowyFormattingLayer: &mut Vec<u8>
FlowyFormattingLayer->>FlowyFormattingLayer: serde_json::Serializer::new(&mut *buf)
FlowyFormattingLayer->>FlowyFormattingLayer: serialize_map(None)
FlowyFormattingLayer->>FlowyFormattingLayer: format_event_message(...)
FlowyFormattingLayer->>FlowyFormattingLayer: serialize_entry(MESSAGE, message)
FlowyFormattingLayer->>FlowyFormattingLayer: map_serializer.end()
FlowyFormattingLayer->>LOG_BUFFER: write_all(b"\n")
FlowyFormattingLayer->>MakeWriter: make_writer()
MakeWriter-->>FlowyFormattingLayer: Writer
FlowyFormattingLayer->>Writer: write_all(&buf)
Writer-->>FlowyFormattingLayer: Result
FlowyFormattingLayer-->>TracingSubscriber: return
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ViewTitleBar,_cachedBreadcrumbsis populated but never actually used by theListView.separated(except indirectly via_buildDeletedTitle), so either wire the cache into the list rendering or remove the caching to avoid unused complexity and potential confusion. - The previous breadcrumb row included
_buildLockPageStatus(context)after the titles, but the newListView.separatedversion no longer renders the lock status; if that indicator is still needed, reintroduce it outside the list so it’s not lost in the refactor. - In
ChatTable::new, the newserialize_rag_ids_from_uuidshelper is defined inside the function and uses multipleunwrap/unwrap_or_defaultcalls; consider lifting it to module scope and handling serialization/UTF-8 errors explicitly instead of potentially panicking or silently returning an empty string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewTitleBar`, `_cachedBreadcrumbs` is populated but never actually used by the `ListView.separated` (except indirectly via `_buildDeletedTitle`), so either wire the cache into the list rendering or remove the caching to avoid unused complexity and potential confusion.
- The previous breadcrumb row included `_buildLockPageStatus(context)` after the titles, but the new `ListView.separated` version no longer renders the lock status; if that indicator is still needed, reintroduce it outside the list so it’s not lost in the refactor.
- In `ChatTable::new`, the new `serialize_rag_ids_from_uuids` helper is defined inside the function and uses multiple `unwrap`/`unwrap_or_default` calls; consider lifting it to module scope and handling serialization/UTF-8 errors explicitly instead of potentially panicking or silently returning an empty string.
## Individual Comments
### Comment 1
<location path="frontend/appflowy_flutter/lib/workspace/presentation/widgets/view_title_bar.dart" line_range="105-112" />
<code_context>
+ // 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,
+ ancestors,
+ state.isDeleted,
+ pageAccessLevelState.isEditable,
+ pageAccessLevelState,
+ );
+ _cachedKey = currentKey;
+ }
</code_context>
<issue_to_address>
**issue:** Breadcrumb caching is computed but not actually used in the ListView builder.
`_cachedBreadcrumbs` and `_cachedKey` are updated, but `itemBuilder` still rebuilds breadcrumbs from `ancestors` each time and never reads `_cachedBreadcrumbs`, so the cache has no effect and only adds complexity. Either use `_cachedBreadcrumbs` to drive `itemCount`/`itemBuilder` (e.g., via precomputed widgets or view models), or remove the caching and let `ListView` handle the work.
</issue_to_address>
### Comment 2
<location path="frontend/rust-lib/lib-log/src/layer.rs" line_range="110" />
<code_context>
+ fn serialize_span<
</code_context>
<issue_to_address>
**suggestion:** Span serialization now performs writing directly, but `emit` still exists and the responsibilities are split awkwardly.
`serialize_span` now both serializes and writes via `LOG_BUFFER`, while `emit` still exists as a separate writer over a `Vec<u8>` but is no longer used for spans (and barely for events). This split makes the API harder to reason about and easy to misuse. It would be clearer to consolidate responsibilities (e.g., keep `serialize_span` pure and let `emit` handle I/O, or the opposite) and remove the now‑redundant path so the logging flow and buffer ownership are unambiguous.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (_cachedKey != currentKey || _cachedBreadcrumbs.isEmpty) { | ||
| _cachedBreadcrumbs = _buildViewTitles( | ||
| context, | ||
| ancestors, | ||
| state.isDeleted, | ||
| pageAccessLevelState.isEditable, | ||
| pageAccessLevelState, | ||
| ); |
There was a problem hiding this comment.
issue: Breadcrumb caching is computed but not actually used in the ListView builder.
_cachedBreadcrumbs and _cachedKey are updated, but itemBuilder still rebuilds breadcrumbs from ancestors each time and never reads _cachedBreadcrumbs, so the cache has no effect and only adds complexity. Either use _cachedBreadcrumbs to drive itemCount/itemBuilder (e.g., via precomputed widgets or view models), or remove the caching and let ListView handle the work.
|
|
||
| // write newline and emit | ||
| b.write_all(b"\n")?; | ||
| self.make_writer.make_writer().write_all(&b) |
There was a problem hiding this comment.
suggestion: Span serialization now performs writing directly, but emit still exists and the responsibilities are split awkwardly.
serialize_span now both serializes and writes via LOG_BUFFER, while emit still exists as a separate writer over a Vec<u8> but is no longer used for spans (and barely for events). This split makes the API harder to reason about and easy to misuse. It would be clearer to consolidate responsibilities (e.g., keep serialize_span pure and let emit handle I/O, or the opposite) and remove the now‑redundant path so the logging flow and buffer ownership are unambiguous.
Motivation
format!calls created transientStringallocations in hot logging code paths.What changed
format!withwrite!into preallocatedStringbuffers and integrated with the thread-local buffer.Performance impact
Testing
cargo checkand unit tests for logging code path.Risk and compatibility
Checklist
Summary by Sourcery
Optimize logging and related data serialization while improving Flutter view title UI performance and updating tooling configuration.
Bug Fixes:
Enhancements:
perfcommit type in commitlint configuration for performance-focused changes.