Improve log rendering performance #150
Conversation
|
Caution Review failedFailed to post review comments WalkthroughReplaced dynamic log storage with a fixed-size ring buffer in src/gui.c, adjusted gui_log/gui_render_log/gui_clear_message_log to use head/tail indices and avoid per-line allocation, added LOG_LINES_MAX_LEN in src/gui.h, and instrumented render_gui in src/main.c with timing blocks around GUI render calls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Code
participant GUI as GUI
participant Ring as LogRingBuffer
participant Tex as TextureCache
participant Ren as Renderer
rect rgb(240,248,255)
note right of GUI: Enqueue log
App->>GUI: gui_log(message)
GUI->>Ring: write at tail index
alt ring full
Ring-->>Ring: advance head (overwrite oldest)
end
Ring-->>GUI: return slot index
end
rect rgb(245,255,240)
note right of GUI: Render cycle (render_gui -> gui_render_log)
App->>GUI: render_gui()
GUI->>GUI: TIME blocks around sub-renders
GUI->>Ring: compare last_head vs head/tail
alt new lines present
GUI->>Tex: generate/update textures for new visible lines
else no new lines
GUI-->>Tex: skip texture regeneration
end
GUI->>Ren: draw visible line textures by computed indices
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gui.c (1)
591-615: Enqueue logic: maintain count and use macro for sizes.Increment count when not full; advance head only on overwrite. Also replace magic 200.
void gui_log(const char *fmt, ...) { - char buffer[200]; + char buffer[LOG_LINES_MAX_LEN]; char *next_message; char tstamp[10]; @@ // Get next log line ptr next_message = log_data.log[log_data.tail]; // Increment indexes - log_data.tail = (uint8_t)((log_data.tail + 1) % log_data.capacity); - if (log_data.tail == log_data.head) { - log_data.head = (uint8_t)(log_data.head + 1) % log_data.capacity; - } + log_data.tail = (uint8_t)((log_data.tail + 1) % log_data.capacity); + if (log_data.tail == log_data.head) { + // Overwriting oldest + log_data.head = (uint8_t)((log_data.head + 1) % log_data.capacity); + } else if (log_data.count < log_data.capacity) { + ++log_data.count; + } // Build log line timestamp(tstamp, 10); va_start(args, fmt); - m_vsprintf(buffer, 200, fmt, args); + m_vsprintf(buffer, LOG_LINES_MAX_LEN, fmt, args); va_end(args); m_sprintf(next_message, log_data.strlen, "%s > %s", tstamp, buffer); }
🧹 Nitpick comments (3)
src/gui.h (1)
24-24: Good addition; align usages to avoid magic numbers.Use this macro in src/gui.c arrays and buffers (currently hardcoded 200) to keep capacities consistent.
src/gui.c (2)
382-436: Replace magic 200 with LOG_LINES_MAX_LEN.Keeps sizes consistent with the new macro.
- char buffer[200]; + char buffer[LOG_LINES_MAX_LEN]; @@ - m_sprintf(buffer, 200, "Dungeon level: %d", map->level); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "Dungeon level: %d", map->level); @@ - m_sprintf(buffer, 200, "x %u", (unsigned int) player->potion_sips); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "x %u", (unsigned int) player->potion_sips); @@ - m_sprintf(buffer, 200, "x %u", (unsigned int) player->daggers); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "x %u", (unsigned int) player->daggers); @@ - m_sprintf(buffer, 200, "x %.2f", player->gold); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "x %.2f", player->gold); @@ - m_sprintf(buffer, 200, "XP: %u / %u", data.current, data.nextLevel); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "XP: %u / %u", data.current, data.nextLevel); @@ - m_sprintf(buffer, 200, "Level: %u", data.level); + m_sprintf(buffer, LOG_LINES_MAX_LEN, "Level: %u", data.level);
49-56: Type nit: prefer size_t for max line length.log_data.strlen holds a byte count; size_t matches snprintf APIs and removes 255-ceiling risk if LOG_LINES_MAX_LEN grows.
- uint8_t strlen; + size_t strlen;Note: adjust the initializer’s type accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui.c(4 hunks)src/gui.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/gui.c (2)
src/util.c (3)
timestamp(161-180)m_vsprintf(75-84)m_sprintf(60-73)src/texture.c (2)
texture_load_from_text(154-189)texture_render(255-262)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Ubuntu (GCC, PROFILER)
- GitHub Check: Mac (Clang)
- GitHub Check: Mac (GCC)
- GitHub Check: Ubuntu (mingw)
- GitHub Check: Ubuntu (Clang)
- GitHub Check: Windows (MSVC)
- GitHub Check: Ubuntu (GCC)
- GitHub Check: Analyze (c-cpp)
LiquidityC
force-pushed
the
improve_log_render_performance
branch
2 times, most recently
from
ebedce9 to
f3af043
Compare
Logs are now stored in a circular buffer rather then a shifting list of strings. This will improve the performance when adding logs but should not have that much impace since we don't do this on every frame. However the rendering of the logs has been improved to reduce the performance impact when generating textures for each log line on every frame.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui.c(4 hunks)src/gui.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gui.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/gui.c (2)
src/util.c (3)
timestamp(161-180)m_vsprintf(75-84)m_sprintf(60-73)src/texture.c (1)
texture_load_from_text(154-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Mac (Clang)
- GitHub Check: Ubuntu (mingw)
- GitHub Check: Mac (GCC)
- GitHub Check: Windows (MSVC)
- GitHub Check: Ubuntu (GCC, PROFILER)
- GitHub Check: Ubuntu (GCC)
- GitHub Check: Ubuntu (Clang)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
src/gui.c (1)
658-667: Subject etcBut instructions say: subject line bold, short. Maybe: "**Head comparison misses new logs after wrap-around**" etc. Then short explanation. Then diff. Let's craft. Need to ensure diff includes definitions. But is there requirement we only modify lines referencing comment? not necessary? but we must keep glimpsed to more lines. But we comment on lines 658-667 (where update occurs). Provide diff with addition near top and in gui_log? but comment referencing these lines; diff may include code outside? instructions? we can include necessary additions even outside line range? Provide extra code block? They say we can generate diff for suggestion; if brackets include modifications outside actual lines, is that allowed? Hard: we can include modifications to relevant lines only (line range 658-667). But to fix we need to add new static variable and update `gui_log`. Should we include diff for all necessary modifications? we can include diff with minimal modifications but includes lines outside? Probably allowed. But they instruct to provide diff code blocks for change suggestions within selected line ranges to highlight. They say "If applicable, demonstrate bias towards action by proactively generating diff code blocks proposing fixes or refactors using + and - marks. Diff code blocks are necessary for clearly highlighting your suggested changes within the selected line ranges." Hmm—they expect diff within same lines. But we need to modify other lines (like `gui_log`). Maybe we can mention in comment and provide diff for necessary modifications. It might include lines outside? Not sure. They say "In addition to diff code blocks for suggesting changes within the selected line ranges, you can also generate additional language-specific code blocks, but only to support changes that need to be applied outside the selected line ranges." So we can include diff covering multiple sections, even though line range same? Possibly Southeastern. But unstoppable. Let's craft diff splitted. Example:@@ -static uint8_t last_head = 0; +static uint32_t last_sequence = 0; @@ - if (log_data.head != last_head) { + if (log_write_sequence != last_sequence) { @@ - last_head = log_data.head; + last_sequence = log_write_sequence;But also need to show addition near top: log_write_sequence static global, and increment in gui_log. For addition outside (line_range not within 658-667). We can provide additional diff in same suggestion? guidelines? maybe we can include entire diff with context. Let's produce diff including modifications to relevant sections. Pseudo:@@ static struct LogData_t { ... } log_data = { ... }; + +static uint32_t log_write_sequence = 0;Another diff for gui_log. Finally diff for render. Maybe we can include multi-block diff. Ok. Now produce final comment. Need to ensure sedation. Let's craft. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Update log textures | ||
| const uint8_t visible = log_data.count; | ||
| if (log_data.head != last_head) { | ||
| for (uint8_t i = 0; i < visible; ++i) { | ||
| const uint8_t log_index = (uint8_t)((log_data.tail + i) % log_data.capacity); | ||
| Texture *t = gui->log_lines[i]; | ||
| texture_load_from_text(t, log_data.log[log_index], C_WHITE, C_BLACK, cam->renderer); | ||
| } | ||
| } | ||
| last_head = log_data.head; |
There was a problem hiding this comment.
Let's craft final comment.
No other issues?
Also ensure there isn't other bug.
Maybe check if (log_data.count > 0 && log_data.head == log_data.tail) { log_data.tail = log_data.tail +1; log_data.count = log_data.capacity; } else if (log_data.count < log_data.capacity) { log_data.count++; } For initial state count=0 head=tail=0. Condition false (count>0 false). else if true -> count=1. Works.
But there is minor: When buffer full, they set count=capacity but no decrement. But count should remain capacity, but they set count to capacity each time (redundant but fine). Might hamper watchers? But fine.
Another issue: log_data.tail != last_tail replaced with head check. Should we consider also glimpsed visible value computed before condition. When visible equals count but if new logs appended and head changed, but count maybe capacity. Works.
But we flagged major bug.
Need to provide diff suggestion using comment.
Let's craft diff: we can't change entire but minimal.
Pseudo diff:
static struct LogData_t {
...
} log_data = { ... };
+static uint32_t log_write_seq = 0;
In gui_log:
...
m_sprintf(next_message,...);
+ log_write_seq++;
In gui_render_log: pre visible? we can update:
- static uint8_t last_head = 0;
+ static uint32_t last_seq = 0;
...
- if (log_data.head != last_head) {
+ if (log_write_seq != last_seq) {
...
- }
- last_head = log_data.head;
+ last_seq = log_write_seq;
+ }
Need to include <stdint.h>? Already uses uint8_t so he includes gui.h maybe. But we add uint32_t. Already using there? yes from stdint probably. This file includes gui.h that may include STD? but we use uint32_t? Already used elsewhere? Search in file: there is uint32_t? maybe not. But we can rely on stdint.h included by gui.h. Probably.
But we propose to use uint32_t. We should mention resetting on clear? Maybe not necessary but degrade? After clear, last_seq remains equal to log_write_seq. With visible=0, we skip updates and nothing rendered. After new log, log_write_seq increments -> update occurs. Works.
But we should mention to reset last_seq? On, we want to ensure we don't overflow or degrade. uint32_t enough. last_seq initial 0. Suppose log_write_seq initial 0. On first logs, increments to 1 etc. render sees difference.
But we must handle when no logs yet: log_write_seq=0, last_seq=0 (since static). On first render before logs, we skip update (makes sense). After first log appended, log_write_seq increments to 1. On render, log_write_seq (1) != last_seq (0) -> update, last_seq=1.
Edge case: log_write_seq wrap-around after 2^32 operations. Accept.
Let's craft comment referencing the bug and propose diff.
Also we must include tag ``.
Let's craft comment.
Need to mention scenario: more than LOG_LINES_COUNT logs between frames.
Ok.
Also must mention abiding instructions: formatting? Provided.
Let's craft final comment.
Need to include patch.
Remember to mention instruct: "Based on ..."? Not referencing guidelines.
Construct comment:
<details>
<summary>🤖 Prompt for AI Agents</summary>
In src/gui.c around lines 658 to 667, the render update check uses
log_data.head/last_head so if more than LOG_LINES_COUNT messages are written
between frames the head can wrap and appear unchanged, skipping needed texture
updates; replace the head-equality check with a monotonically-incremented write
sequence counter (incremented whenever a new log is appended) and compare that
counter to a static last_seen counter in the render path so any number of new
messages between frames triggers a texture refresh, updating last_seen after
copying textures.
</details>
<!-- fingerprinting:phantom:medusa:sabertoothed -->
<!-- This is an auto-generated comment by CodeRabbit -->
Logs are now stored in a circular buffer rather then a shifting list of
strings. This will improve the performance when adding logs but should
not have that much impace since we don't do this on every frame.
However the rendering of the logs has been improved to reduce the
performance impact when generating textures for each log line on every
frame.
Benchmarking
Issue
Fixes: #149
Summary by CodeRabbit
New Features
Improvements