Render summary benchmark results as Markdown tables#621
Conversation
Signed-off-by: reflecttypefor <reflecttypefor@outlook.com>
📝 WalkthroughWalkthroughAdds ChangesBenchmark Markdown Report Helpers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Markdown table reporting for benchmarks by adding a new bench_report module, updating the benchmark summary template to output Markdown tables, and adding corresponding unit tests. The feedback highlights an issue where the global printedMarkdownHeaders variable is never cleared, which can lead to broken Markdown formatting when multiple separate tables with identical headers are printed in a single run. A helper function to reset this state is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| proc reportMarkdownTable*(headers, cells: openArray[string]) = | ||
| doAssert headers.len == cells.len | ||
|
|
||
| let key = headers.join("\t") | ||
| for printedHeader in printedMarkdownHeaders: | ||
| if printedHeader == key: | ||
| echo markdownTableRow(cells) | ||
| return | ||
|
|
||
| echo markdownTableHeader(headers) | ||
| echo markdownTableSeparator(headers.len) | ||
| printedMarkdownHeaders.add key | ||
| echo markdownTableRow(cells) |
There was a problem hiding this comment.
Because printedMarkdownHeaders is a global variable that is never cleared, any subsequent table with the same headers in the same benchmark run will omit the header and separator lines. This will result in broken Markdown rendering if there is intervening text or if they are meant to be separate tables (e.g., when benchmarking different curves or categories in a single run).
To support multiple separate tables with identical headers, we should expose a helper function to reset/clear the printed headers tracker.
proc clearPrintedHeaders*() =
## Clear the list of printed headers to allow starting a new table
## with the same headers.
printedMarkdownHeaders.setLen(0)
proc reportMarkdownTable*(headers, cells: openArray[string]) =
doAssert headers.len == cells.len
let key = headers.join("\t")
for printedHeader in printedMarkdownHeaders:
if printedHeader == key:
echo markdownTableRow(cells)
return
echo markdownTableHeader(headers)
echo markdownTableSeparator(headers.len)
printedMarkdownHeaders.add key
echo markdownTableRow(cells)
Greptile SummaryThis PR adds a
Confidence Score: 4/5The change is additive and limited to benchmark output formatting; no production cryptographic code is touched, making it safe to merge. The core formatting logic is straightforward and the integration into bench_summary_template is a clean mechanical replacement. The main rough edges are in bench_report.nim: the module-level printedMarkdownHeaders has no reset path, doAssert is stripped by -d:danger so a header/cell length mismatch would silently corrupt output under that build flag, and the test suite misses the backslash-escaping path. None of these affect correctness of the cryptographic library itself. benchmarks/bench_report.nim and tests/t_bench_report.nim warrant a second look — the global state design and incomplete test coverage are the areas most likely to cause surprises as the helper is extended to other benchmark suites. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bench_summary_template.nim\nreport()"] -->|"SupportsGetTicks = true"| B["reportMarkdownTable\n5 columns"]
A -->|"SupportsGetTicks = false"| C["reportMarkdownTable\n4 columns"]
B --> D["bench_report.nim\nreportMarkdownTable()"]
C --> D
D --> E{"headers.join(tab)\nalready in\nprintedMarkdownHeaders?"}
E -->|"No (first call)"| F["echo markdownTableHeader\necho markdownTableSeparator\nadd key to global seq\necho markdownTableRow"]
E -->|"Yes (subsequent calls)"| G["echo markdownTableRow only"]
F --> H["Markdown table output"]
G --> H
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["bench_summary_template.nim\nreport()"] -->|"SupportsGetTicks = true"| B["reportMarkdownTable\n5 columns"]
A -->|"SupportsGetTicks = false"| C["reportMarkdownTable\n4 columns"]
B --> D["bench_report.nim\nreportMarkdownTable()"]
C --> D
D --> E{"headers.join(tab)\nalready in\nprintedMarkdownHeaders?"}
E -->|"No (first call)"| F["echo markdownTableHeader\necho markdownTableSeparator\nadd key to global seq\necho markdownTableRow"]
E -->|"Yes (subsequent calls)"| G["echo markdownTableRow only"]
F --> H["Markdown table output"]
G --> H
Reviews (1): Last reviewed commit: "Render summary benchmark results as Mark..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test "escapes Markdown table cells": | ||
| check markdownTableRow(["A | B", "line 1\nline 2"]) == "| A \\| B | line 1<br>line 2 |" |
There was a problem hiding this comment.
The test exercises pipe (|) and newline (\n) escaping but not the backslash path. markdownEscape replaces \ with \\ first in the multiReplace chain — if that path regresses (e.g., the order of replacements is changed), no existing test would catch it. Adding check markdownTableRow(["a\\b"]) == "| a\\\\b |" would cover this case.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| proc reportMarkdownTable*(headers, cells: openArray[string]) = | ||
| doAssert headers.len == cells.len | ||
|
|
||
| let key = headers.join("\t") | ||
| for printedHeader in printedMarkdownHeaders: | ||
| if printedHeader == key: | ||
| echo markdownTableRow(cells) | ||
| return | ||
|
|
||
| echo markdownTableHeader(headers) | ||
| echo markdownTableSeparator(headers.len) | ||
| printedMarkdownHeaders.add key | ||
| echo markdownTableRow(cells) |
There was a problem hiding this comment.
printedMarkdownHeaders is a module-level var with no reset mechanism. If reportMarkdownTable is called from two logically separate benchmark sections in the same process run that happen to share identical column headers (e.g., two different curve suites both using the 5-column schema), the second group's rows will be silently appended to the first table with no header row — producing valid Nim output but broken Markdown. Exposing a resetMarkdownHeaders*() proc (or accepting a tableId discriminant) would give callers control without breaking the current single-table use case.
| result.add " " & markdownEscape(cell) & " |" | ||
|
|
||
| proc reportMarkdownTable*(headers, cells: openArray[string]) = | ||
| doAssert headers.len == cells.len |
There was a problem hiding this comment.
doAssert is stripped by -d:danger
doAssert headers.len == cells.len is removed at compile time when the project is built with -d:danger. Benchmarks are often compiled with -d:danger for accurate timings. When the check is absent and the arrays are mismatched, markdownTableRow silently produces a short row, corrupting the table without any diagnostic. A runtime check that raises a ValueError would be more robust and survive all build modes.
| # KDF | ||
| # ---------------------------------------------------------- | ||
| ("tests/t_kdf_hkdf.nim", false), | ||
| ("tests/t_bench_report.nim", false), |
There was a problem hiding this comment.
t_bench_report.nim is placed immediately after t_kdf_hkdf.nim inside the # KDF block. It would be clearer in a dedicated # Benchmarks or # Utilities section — or appended after the current last entry — so that future readers don't associate it with key-derivation tests.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
benchmarks/bench_report.nim (2)
39-39: 💤 Low valueCache key could collide if headers contain tab characters.
The cache uses
headers.join("\t")as the key. If a header cell contains a literal tab character, different header arrays could produce the same key (e.g.,["A\tB"]vs["A", "B"]).🛡️ Alternative: Use a delimiter that's already escaped
Since
markdownEscapeconverts newlines to<br>, you could use\nas the delimiter:- let key = headers.join("\t") + let key = headers.join("\n")Or apply escaping before joining to guarantee uniqueness:
- let key = headers.join("\t") + let key = headers.mapIt(markdownEscape(it)).join("\t")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/bench_report.nim` at line 39, The cache key generation using headers.join("\t") is vulnerable to collisions when header cells contain literal tab characters, as different header arrays like ["A\tB"] and ["A", "B"] would produce identical keys. Replace the tab character delimiter with a character that is guaranteed not to appear in the header values, such as newline character "\n" since newlines are already being converted to "<br>" by markdownEscape. Alternatively, apply escaping to the headers before joining them to ensure the delimiter cannot be confused with actual header content.
40-43: ⚡ Quick winLinear search could impact performance with many distinct table formats.
The cache lookup performs a linear search through all previously seen headers. If benchmarks generate many different table formats, this O(n) lookup could become a bottleneck.
⚡ Use HashSet for O(1) lookup
-var printedMarkdownHeaders: seq[string] +import std/sets + +var printedMarkdownHeaders: HashSet[string] proc reportMarkdownTable*(headers, cells: openArray[string]) = doAssert headers.len == cells.len let key = headers.join("\t") - for printedHeader in printedMarkdownHeaders: - if printedHeader == key: - echo markdownTableRow(cells) - return + if key in printedMarkdownHeaders: + echo markdownTableRow(cells) + return echo markdownTableHeader(headers) echo markdownTableSeparator(headers.len) - printedMarkdownHeaders.add key + printedMarkdownHeaders.incl key echo markdownTableRow(cells)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/bench_report.nim` around lines 40 - 43, Replace the linear search through printedMarkdownHeaders with a HashSet data structure. Convert printedMarkdownHeaders from a sequence/list to a HashSet that stores the header keys that have already been printed. Instead of the for loop that iterates through all elements checking equality with key, use a direct membership check (contains operation) on the HashSet to achieve O(1) lookup time. Update both the condition check and the insertion point where new headers are added to the set.tests/t_bench_report.nim (1)
13-19: ⚡ Quick winConsider expanding test coverage for edge cases and caching behavior.
The current tests verify basic formatting but miss several important scenarios:
- Backslash escaping: Test that
\becomes\\- Carriage return removal: Test that
\ris removed- Caching behavior: Call
reportMarkdownTabletwice with the same headers to verify the header is only printed once- Length mismatch: Verify
doAsserttriggers when headers.len != cells.len- Empty cells: Test behavior with empty strings
📋 Suggested additional tests
test "escapes backslashes and handles carriage returns": check markdownEscape("a\\b\rc") == "a\\\\bc" test "caching behavior prints header only once": # This would require capturing stdout or refactoring to return strings # Alternatively, test the cache directly if exposed discard test "empty cells are handled": check markdownTableRow(["", "data"]) == "| | data |"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/t_bench_report.nim` around lines 13 - 19, Expand the test suite to cover additional edge cases for the markdown report functions. Add test cases for: backslash escaping in the markdownEscape function to verify that backslashes are properly doubled; carriage return handling to confirm that \r characters are removed; empty cells behavior in markdownTableRow to ensure empty strings are handled correctly; and a length mismatch test to verify that the doAssert in reportMarkdownTable or related functions properly triggers when headers.len does not equal cells.len. Additionally, consider adding a test for the caching behavior of reportMarkdownTable by calling it twice with the same headers and verifying the header is only printed once, though this may require capturing stdout or exposing the cache for direct testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/bench_report.nim`:
- Line 11: The module-level variable `printedMarkdownHeaders` is not thread-safe
and will grow unboundedly during long-running processes, potentially causing
issues in concurrent benchmarking scenarios. To fix this, apply the
`{.threadvar.}` pragma annotation to `printedMarkdownHeaders` to make it
thread-local storage, ensuring each thread maintains its own separate cache
instance and eliminating both the thread-safety concern and unbounded growth
issue.
---
Nitpick comments:
In `@benchmarks/bench_report.nim`:
- Line 39: The cache key generation using headers.join("\t") is vulnerable to
collisions when header cells contain literal tab characters, as different header
arrays like ["A\tB"] and ["A", "B"] would produce identical keys. Replace the
tab character delimiter with a character that is guaranteed not to appear in the
header values, such as newline character "\n" since newlines are already being
converted to "<br>" by markdownEscape. Alternatively, apply escaping to the
headers before joining them to ensure the delimiter cannot be confused with
actual header content.
- Around line 40-43: Replace the linear search through printedMarkdownHeaders
with a HashSet data structure. Convert printedMarkdownHeaders from a
sequence/list to a HashSet that stores the header keys that have already been
printed. Instead of the for loop that iterates through all elements checking
equality with key, use a direct membership check (contains operation) on the
HashSet to achieve O(1) lookup time. Update both the condition check and the
insertion point where new headers are added to the set.
In `@tests/t_bench_report.nim`:
- Around line 13-19: Expand the test suite to cover additional edge cases for
the markdown report functions. Add test cases for: backslash escaping in the
markdownEscape function to verify that backslashes are properly doubled;
carriage return handling to confirm that \r characters are removed; empty cells
behavior in markdownTableRow to ensure empty strings are handled correctly; and
a length mismatch test to verify that the doAssert in reportMarkdownTable or
related functions properly triggers when headers.len does not equal cells.len.
Additionally, consider adding a test for the caching behavior of
reportMarkdownTable by calling it twice with the same headers and verifying the
header is only printed once, though this may require capturing stdout or
exposing the cache for direct testing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9ae4e34-eaad-4bfa-9fbd-e2ab33e02399
📒 Files selected for processing (4)
benchmarks/bench_report.nimbenchmarks/bench_summary_template.nimconstantine.nimbletests/t_bench_report.nim
|
|
||
| import std/strutils | ||
|
|
||
| var printedMarkdownHeaders: seq[string] |
There was a problem hiding this comment.
Thread-safety concern and unbounded memory growth.
The module-level printedMarkdownHeaders cache is not thread-safe and will grow unboundedly in long-running processes. If benchmarks are run concurrently or the process executes many different benchmark suites, this could cause issues.
Consider either:
- Using
{.threadvar.}if each thread should have its own cache, or - Adding explicit cache management (e.g., a
clearMarkdownCache()proc), or - Using a
HashSet[string]with a maximum size limit
🔒 Example: Thread-local cache
-var printedMarkdownHeaders: seq[string]
+var printedMarkdownHeaders {.threadvar.}: seq[string]Or with explicit management:
var printedMarkdownHeaders: seq[string]
+
+proc clearMarkdownCache*() =
+ printedMarkdownHeaders.setLen(0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var printedMarkdownHeaders: seq[string] | |
| var printedMarkdownHeaders {.threadvar.}: seq[string] |
| var printedMarkdownHeaders: seq[string] | |
| var printedMarkdownHeaders: seq[string] | |
| proc clearMarkdownCache*() = | |
| printedMarkdownHeaders.setLen(0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/bench_report.nim` at line 11, The module-level variable
`printedMarkdownHeaders` is not thread-safe and will grow unboundedly during
long-running processes, potentially causing issues in concurrent benchmarking
scenarios. To fix this, apply the `{.threadvar.}` pragma annotation to
`printedMarkdownHeaders` to make it thread-local storage, ensuring each thread
maintains its own separate cache instance and eliminating both the thread-safety
concern and unbounded growth issue.
|
Thank you for the PR. I think the next step would be to modify 2 benchmark files as a branch that build on this branch and a "staggered" PR that targets this PR branch so we can see what it looks like. I suggest:
as they are very standardized while some are somewhat customized and would need more work (https://github.com/mratsim/constantine/blob/master/benchmarks/bench_eth_evm_precompiles.nim) You can run them with |
2 participants
Summary
This adds a small Markdown table reporting helper for benchmark output and uses it in the summary benchmark template.
The summary benchmark output can now be copied directly into GitHub, ethresear.ch, and ethereum-magicians.org without losing alignment due to line wrapping.
This is a first focused step toward #522, keeping the change limited to the summary benchmarks before expanding the same helper to other benchmark suites.
Changes
benchmarks/bench_report.nimwith Markdown table formatting helpersbench_summary_templatebenchmark rows as Markdown tablesconstantine.nimbleSummary by CodeRabbit
New Features
Tests
Chores