Skip to content

Render summary benchmark results as Markdown tables #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
reflecttypefor wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Render summary benchmark results as Markdown tables #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions benchmarks/bench_report.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Constantine
# Copyright (c) 2018-2019 Status Research & Development GmbH
# Copyright (c) 2020-Present Mamy Andre-Ratsimbazafy
# Licensed and distributed under either of
# * MIT license (license terms in the root directory or at http://opensource.org/licenses/MIT).
# * Apache v2 license (license terms in the root directory or at http://www.apache.org/licenses/LICENSE-2.0).
# at your option. This file may not be copied, modified, or distributed except according to those terms.

import std/strutils

var printedMarkdownHeaders: seq[string]

@coderabbitai coderabbitai Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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:

  1. Using {.threadvar.} if each thread should have its own cache, or
  2. Adding explicit cache management (e.g., a clearMarkdownCache() proc), or
  3. 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.

Suggested change
var printedMarkdownHeaders: seq[string]
var printedMarkdownHeaders {.threadvar.}: seq[string]
Suggested change
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.


func markdownEscape*(cell: string): string =
cell.multiReplace(
("\\", "\\\\"),
("|", "\\|"),
("\r", ""),
("\n", "<br>")
)

func markdownTableHeader*(headers: openArray[string]): string =
result = "|"
for header in headers:
result.add " " & markdownEscape(header) & " |"

func markdownTableSeparator*(columns: int): string =
result = "|"
for _ in 0 ..< columns:
result.add " --- |"

func markdownTableRow*(cells: openArray[string]): string =
result = "|"
for cell in cells:
result.add " " & markdownEscape(cell) & " |"

proc reportMarkdownTable*(headers, cells: openArray[string]) =
doAssert headers.len == cells.len

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.


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)
Comment on lines +36 to +48

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)

Comment on lines +36 to +48

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No API to start a fresh table; global state silently merges output across suites

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.

11 changes: 9 additions & 2 deletions benchmarks/bench_summary_template.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import
constantine/hash_to_curve/hash_to_curve,
# Helpers
helpers/prng_unsafe,
./bench_report,
./bench_blueprint

export
Expand All @@ -45,9 +46,15 @@ proc report(op, domain: string, start, stop: MonoTime, startClk, stopClk: int64,
let ns = inNanoseconds((stop-start) div iters)
let throughput = 1e9 / float64(ns)
when SupportsGetTicks:
echo &"{op:<35} {domain:<40} {throughput:>15.3f} ops/s {ns:>9} ns/op {(stopClk - startClk) div iters:>9} CPU cycles (approx)"
reportMarkdownTable(
["Operation", "Domain", "Throughput", "Latency", "CPU cycles (approx)"],
[op, domain, &"{throughput:.3f} ops/s", &"{ns} ns/op", $((stopClk - startClk) div iters)]
)
else:
echo &"{op:<35} {domain:<40} {throughput:>15.3f} ops/s {ns:>9} ns/op"
reportMarkdownTable(
["Operation", "Domain", "Throughput", "Latency"],
[op, domain, &"{throughput:.3f} ops/s", &"{ns} ns/op"]
)

macro fixEllipticDisplay(T: typedesc): untyped =
# At compile-time, enums are integers and their display is buggy
Expand Down
1 change: 1 addition & 0 deletions constantine.nimble
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,7 @@ const testDesc: seq[tuple[path: string, useGMP: bool]] = @[
# KDF
# ----------------------------------------------------------
("tests/t_kdf_hkdf.nim", false),
("tests/t_bench_report.nim", false),

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test registered in the wrong section

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!


# Primitives
# ----------------------------------------------------------
Expand Down
19 changes: 19 additions & 0 deletions tests/t_bench_report.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Constantine
# Copyright (c) 2018-2019 Status Research & Development GmbH
# Copyright (c) 2020-Present Mamy Andre-Ratsimbazafy
# Licensed and distributed under either of
# * MIT license (license terms in the root directory or at http://opensource.org/licenses/MIT).
# * Apache v2 license (license terms in the root directory or at http://www.apache.org/licenses/LICENSE-2.0).
# at your option. This file may not be copied, modified, or distributed except according to those terms.

import
std/unittest,
../benchmarks/bench_report

suite "Benchmark Markdown reports":
test "renders a Markdown table header":
check markdownTableHeader(["Operation", "Domain"]) == "| Operation | Domain |"
check markdownTableSeparator(2) == "| --- | --- |"

test "escapes Markdown table cells":
check markdownTableRow(["A | B", "line 1\nline 2"]) == "| A \\| B | line 1<br>line 2 |"
Comment on lines +18 to +19

@greptile-apps greptile-apps Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing escape coverage for backslashes

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!