perf(logging): reuse thread-local buffer for JSON log serialization#8730
Open
danteboe wants to merge 5 commits into
Open
perf(logging): reuse thread-local buffer for JSON log serialization#8730danteboe wants to merge 5 commits into
danteboe wants to merge 5 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>
Contributor
Reviewer's GuideOptimizes JSON logging performance by reusing a thread-local buffer in Rust, adds caching for Flutter view title breadcrumbs, streamlines UUID list serialization in chat persistence, and updates commit message types while removing several unused configuration/assets files. Sequence diagram for thread-local JSON logging buffer reusesequenceDiagram
participant Application
participant FlowyFormattingLayer
participant LOG_BUFFER
participant MakeWriter
participant Writer
Application->>FlowyFormattingLayer: on_event(event, ctx)
FlowyFormattingLayer->>LOG_BUFFER: with(closure)
activate LOG_BUFFER
LOG_BUFFER-->>FlowyFormattingLayer: borrow_mut()
deactivate LOG_BUFFER
FlowyFormattingLayer->>FlowyFormattingLayer: clear buffer
FlowyFormattingLayer->>FlowyFormattingLayer: serde_json::Serializer::new(buffer)
FlowyFormattingLayer->>FlowyFormattingLayer: serialize_map / serialize_entry
FlowyFormattingLayer->>FlowyFormattingLayer: buffer.write_all("\n")
FlowyFormattingLayer->>MakeWriter: make_writer()
MakeWriter-->>FlowyFormattingLayer: Writer
FlowyFormattingLayer->>Writer: write_all(buffer)
Writer-->>FlowyFormattingLayer: Result
FlowyFormattingLayer-->>Application: return
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 1 issue, and left some high level feedback:
- In the Rust logging layer,
LOG_BUFFERis borrowed mutably for the entire serialization in both the span and event paths while callingtracing::debug!inside that scope; if those debug logs hit the same layer they can re-enterLOG_BUFFER.withand trigger aRefCellborrow panic—consider removing or deferring those debug logs, or separating the reserved-field logging from the hot path. - Now that span and event formatting write directly to the writer using the thread-local buffer, the
emit(&self, Vec<u8>)helper appears unused and could be removed or repurposed to avoid having two different emission paths. - In
ChatTable::new, the nestedserialize_rag_ids_from_uuidshelper is recreated on every call; consider hoisting it to a private function or method (and potentially usingserde_json::to_stringwith a wrapper type) to avoid per-call function definition overhead and to keep serialization logic reusable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Rust logging layer, `LOG_BUFFER` is borrowed mutably for the entire serialization in both the span and event paths while calling `tracing::debug!` inside that scope; if those debug logs hit the same layer they can re-enter `LOG_BUFFER.with` and trigger a `RefCell` borrow panic—consider removing or deferring those debug logs, or separating the reserved-field logging from the hot path.
- Now that span and event formatting write directly to the writer using the thread-local buffer, the `emit(&self, Vec<u8>)` helper appears unused and could be removed or repurposed to avoid having two different emission paths.
- In `ChatTable::new`, the nested `serialize_rag_ids_from_uuids` helper is recreated on every call; consider hoisting it to a private function or method (and potentially using `serde_json::to_string` with a wrapper type) to avoid per-call function definition overhead and to keep serialization logic reusable.
## Individual Comments
### Comment 1
<location path="frontend/rust-lib/flowy-ai-pub/src/persistence/chat_sql.rs" line_range="43-44" />
<code_context>
+ String::from_utf8(buf).unwrap_or_default()
+ }
+
let metadata = serialize_chat_metadata(&metadata);
- let rag_ids = Some(serialize_rag_ids(&rag_ids));
+ let rag_ids = Some(serialize_rag_ids_from_uuids(&rag_ids));
Self {
chat_id,
</code_context>
<issue_to_address>
**suggestion:** New `serialize_rag_ids_from_uuids` duplicates existing serialization logic instead of reusing it.
The new helper manually builds JSON and is only used here, while `serialize_rag_ids` already centralizes this logic. This creates two code paths for the same behavior and risks inconsistencies.
Please either reuse `serialize_rag_ids` by generalizing it to work with `Uuid` (or `Display`), or colocate both helpers and have one delegate to the other so the JSON format is defined in a single place.
Suggested implementation:
```rust
// Convert UUIDs to strings and delegate to the existing serializer
let rag_id_strings: Vec<String> = rag_ids.iter().map(|id| id.to_string()).collect();
serialize_rag_ids(&rag_id_strings)
}
let metadata = serialize_chat_metadata(&metadata);
let rag_ids = Some(serialize_rag_ids_from_uuids(&rag_ids));
```
1. Ensure the signature of `serialize_rag_ids_from_uuids` matches this implementation (e.g. `fn serialize_rag_ids_from_uuids(rag_ids: &[Uuid]) -> String` or similar).
2. Confirm the existing `serialize_rag_ids` function accepts `&[String]` or `&Vec<String>`; if it expects an owned `Vec<String>`, remove the reference (`serialize_rag_ids(rag_id_strings)`).
3. If `serialize_rag_ids` currently has a different parameter type, adjust either its parameter type or add a small adapter so it can accept `&[String]` without changing its behavior.
</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
43
to
+44
| let metadata = serialize_chat_metadata(&metadata); | ||
| let rag_ids = Some(serialize_rag_ids(&rag_ids)); | ||
| let rag_ids = Some(serialize_rag_ids_from_uuids(&rag_ids)); |
Contributor
There was a problem hiding this comment.
suggestion: New serialize_rag_ids_from_uuids duplicates existing serialization logic instead of reusing it.
The new helper manually builds JSON and is only used here, while serialize_rag_ids already centralizes this logic. This creates two code paths for the same behavior and risks inconsistencies.
Please either reuse serialize_rag_ids by generalizing it to work with Uuid (or Display), or colocate both helpers and have one delegate to the other so the JSON format is defined in a single place.
Suggested implementation:
// Convert UUIDs to strings and delegate to the existing serializer
let rag_id_strings: Vec<String> = rag_ids.iter().map(|id| id.to_string()).collect();
serialize_rag_ids(&rag_id_strings)
}
let metadata = serialize_chat_metadata(&metadata);
let rag_ids = Some(serialize_rag_ids_from_uuids(&rag_ids));- Ensure the signature of
serialize_rag_ids_from_uuidsmatches this implementation (e.g.fn serialize_rag_ids_from_uuids(rag_ids: &[Uuid]) -> Stringor similar). - Confirm the existing
serialize_rag_idsfunction accepts&[String]or&Vec<String>; if it expects an ownedVec<String>, remove the reference (serialize_rag_ids(rag_id_strings)). - If
serialize_rag_idscurrently has a different parameter type, adjust either its parameter type or add a small adapter so it can accept&[String]without changing its behavior.
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
thread_local!LOG_BUFFER: RefCell<Vec<u8>>and rewrote serialization to reuse that buffer per-thread, clearing it between uses and writing JSON directly into it.Performance impact
Testing
cargo check/cargo clippyand sample workload.Risk and compatibility
Vec.Checklist
Summary by Sourcery
Optimize logging and related code paths to reduce allocations and improve performance while adjusting UI breadcrumbs caching and housekeeping configs/assets.
New Features:
perfcommit type in commit linting rules.Bug Fixes:
Enhancements:
Vec<String>allocations when serializing RAG IDs in chat persistence by streaming UUIDs directly into a JSON serializer.Chores: