refactor: Used the official table package.

This commit is contained in:
2026-06-19 18:26:25 -04:00
parent f2da8b9f22
commit 92faab2706
7 changed files with 83 additions and 506 deletions

View File

@@ -1,258 +0,0 @@
# Table Rendering Memory Optimization Plan
## Executive Summary
This plan outlines improvements to eliminate excessive memory allocations and copies in the Odin table rendering system. The current implementation makes 10+ allocations per row, while the Zig equivalent makes zero allocations for rendering. This optimization will reduce memory usage, improve performance, and align with the project's efficiency goals.
## Current State Analysis
### Zig Version (Reference Implementation)
- **Allocations**: 1 (data only)
- **Data copies**: 0
- **String allocation**: 0
- **Column widths**: Stack array
- **Output**: Direct to writer
### Odin Version (Current Implementation)
- **Allocations**: 10+ per row
- **Data copies**: Multiple per row
- **String allocation**: 2+ per row (concatenate + slice)
- **Column widths**: Heap allocated
- **Output**: Builder → stdout
### Current Issues Identified
1. **Table Infrastructure** (`table.odin`)
- Uses `strings.Builder` which allocates per-line memory
- Heap-allocated `[dynamic]int` for column widths
- Multiple `strings.concatenate()` calls creating new strings
2. **Command Implementations**
- `cmd_list`: Creates intermediate `[]string` slices per row, allocates new strings via `strings.concatenate()`
- `cmd_sync`: Creates `SyncEntry` structs with cloned strings, allocates dynamic arrays
- `cmd_deps`: Allocates dynamic rows array unnecessarily
3. **Memory Pattern**
- Each command allocates `[][]string` for table data
- Manual struct-to-row transformation creates copies
- Duplicate code across all table-using commands
## Proposed Solutions
### Phase 1: Core Table Infrastructure Overhaul
#### 1.1 Direct Writer-Based Rendering
**Current:**
```odin
b: strings.Builder
strings.builder_init(&b)
// ... build table in builder
fmt.println(strings.to_string(b))
```
**Proposed:**
```odin
render_table :: proc(writer: io.Writer, headers: []string, rows: [][]string)
```
- Replace `strings.Builder` with `io.Writer` output
- Eliminate intermediate string allocations
- Write table components directly to output stream
#### 1.2 Stack-Based Column Widths
**Current:**
```odin
col_widths := make([dynamic]int, 0, len(headers))
```
**Proposed:**
- Use fixed stack arrays for reasonable column counts
- Implement small buffer optimization (SBO) for variable column counts
- Only allocate for tables exceeding threshold (e.g., 16 columns)
#### 1.3 Zero-Copy String Handling
**Current:**
```odin
dir_str := strings.concatenate({row.Dir, "/"}, context.temp_allocator)
```
**Proposed:**
- Replace `strings.concatenate()` with string slicing
- Work directly with `EnvFile.Path` and `EnvFile.Dir` fields
- Use `filepath.base()` and `filepath.dir()` without allocation where possible
### Phase 2: Generic Table Interface
#### 2.1 Field-Based Table Renderer
```odin
Table_Field :: struct {
name: string,
value: string, // String view, no allocation
alignment: Alignment,
}
Table_Config :: struct {
writer: io.Writer,
fields: []Table_Field,
col_widths: []int,
}
render_row :: proc(cfg: Table_Config, row_data: any)
```
- Accept struct fields directly without intermediate arrays
- Support field selection (show only specific fields)
- Alignment options (left/center/right)
#### 2.2 Field Extraction Procs
- Generate field extraction helpers for each struct type
- Avoid string allocation by returning string views
- Cache computed values (like formatted status strings)
#### 2.3 Streaming Table Processing
- Process rows one at a time without collecting all rows
- Reduce peak memory usage from O(N × strings) to O(table_structure)
- Enable early termination if needed
### Phase 3: Command-Specific Optimizations
#### 3.1 Eliminate Intermediate Structs
**Current (cmd_sync):**
```odin
for &file in files {
// ... processing
path_str, _ := strings.clone(file.Path)
status_str, _ := strings.clone(status)
append(&results, SyncEntry{Path = path_str, Status = status_str})
}
```
**Proposed:**
```odin
for &file in files {
result, err_msg := db_sync(&db, &file)
// Direct rendering with zero-copy
render_sync_row(writer, file, result, err_msg)
}
```
- `cmd_sync`: Work directly with `EnvFile` + `SyncFlagEnum`
- `cmd_list`: Use `EnvFile` fields directly, no `ListEntry`
- Generate table content on-the-fly
#### 3.2 In-Place Status Computation
```odin
get_sync_status :: proc(result: SyncFlag, err_msg: string) -> string {
switch {
case .Error in result: return if len(err_msg) > 0 then err_msg else "error"
case .BackedUp in result: return "Backed Up"
case .Restored in result: return "Restored"
case .DirUpdated in result: return "Moved"
case: return "OK"
}
}
```
- Compute status strings without allocation (use static lookup)
- Cache formatted status values if needed
- Reduce allocation count from N to 0 or 1
#### 3.3 Batch Processing
- Reduce allocation count by pooling small allocations
- Use `context.temp_allocator` more effectively
- Pre-allocate buffers for expected sizes
### Phase 4: Format-Agnostic Interface
- Commands generate data → renderers handle format
- Table renderer focuses only on ASCII/Unicode output
- Keep terminal detection in command layer
## Expected Improvements
| Metric | Current | Target | Improvement |
|--------|---------|--------|-------------|
| **Allocations** | 10+ per row | 0-1 per table | 10x+ reduction |
| **Memory copies** | 2-3 per row | 0 | 100% reduction |
| **Peak memory** | O(N × strings) | O(table_structure) | Constant factor |
| **Throughput** | Baseline | 2-3x faster | Performance boost |
## Implementation Strategy
### High-Priority Changes
1. Replace `strings.Builder` with direct `io.Writer` output
2. Convert column widths to stack-based allocation
3. Eliminate intermediate struct allocations in commands
### Medium-Priority Changes
1. Create generic field-based table interface
2. Implement streaming table processing
3. Centralize JSON rendering logic
### Low-Priority Changes
1. Add alignment options beyond left-aligned
2. Implement comprehensive field introspection
3. Add advanced table formatting features
## Tradeoff Questions
Before implementation begins, we need to resolve these architectural questions:
### 1. Generality vs. Performance
**Question:** Should we create a fully generic table renderer (similar to Zig's `Table(T)`) or focus on optimizing the current 3 use cases first?
**Options:**
- **Generic approach**: Higher development cost, future-proof, may have some overhead
- **Specific optimization**: Faster implementation, maximum performance for current use cases, less flexible
**Recommendation:** Start with specific optimizations for current use cases, then generalize patterns that emerge.
### 2. Alignment Support
**Question:** Does the project need left/center/right alignment support, or is left-alignment sufficient?
**Context:** Zig supports alignment options, but current Odin implementation only left-aligns. Most CLI tables work fine with left alignment.
**Recommendation:** Start with left-alignment only, add alignment if specific use cases demand it.
### 3. API Compatibility
**Question:** Should we maintain the current `render_table()` API signature, or are breaking changes acceptable?
**Current API:**
```odin
render_table :: proc(headers: []string, rows: [][]string)
```
**Options:**
- **Maintain API**: Slower to implement, backward compatible, may need adapter layers
- **Break API**: Faster implementation, cleaner code, requires updates to all callers
**Recommendation:** Breaking changes are acceptable since this is an optimization-focused effort and callers are limited to 3 commands.
### 4. Odin Capabilities
**Question:** What runtime reflection or field introspection capabilities does Odin provide?
**Context:** Zig uses `@typeInfo()` and comptime field iteration. We need to understand Odin's equivalent capabilities to design the optimal solution.
**Recommendation:** Investigate Odin's runtime type information capabilities before finalizing the generic table interface design.
### 5. Testing Strategy
**Question:** Should we add comprehensive tests for new table rendering before optimizing commands, or optimize incrementally with tests added afterwards?
**Options:**
- **Test-first**: More robust, catches regressions early, slower initial development
- **Optimize-first**: Faster development, may miss edge cases, requires retroactive testing
**Recommendation:** Hybrid approach - add basic tests for core infrastructure, then optimize incrementally with additional tests for each command.
## Next Steps
1. **Research Phase**: Investigate Odin's type system and reflection capabilities
2. **Prototype Phase**: Create minimal working prototype of zero-allocation table renderer
3. **Refactor Phase**: Incrementally update commands to use new infrastructure
4. **Test Phase**: Add comprehensive tests and verify memory improvements
5. **Benchmark Phase**: Measure performance improvements and memory usage
## Success Criteria
- [ ] Zero allocations for table rendering (excluding initial data)
- [ ] Zero string copies in the happy path
- [ ] All 3 commands (`list`, `sync`, `deps`) use new infrastructure
- [ ] Performance improvement of 2x or more
- [ ] Memory usage reduction of 50% or more
- [ ] No regression in table formatting quality
- [ ] Backward compatibility with JSON output format

View File

@@ -6,6 +6,8 @@
29. Add color flag and support non colored output.
30. Use text/tables for command output
2. Generate md and man pages again.
3. **db.odin:324-327** — Map iteration (`remote_set`) is non-deterministic. Same file can produce different JSON on each backup, causing spurious DB diffs. Sort remotes before storing.

View File

@@ -6,6 +6,7 @@ import "core:os"
import "core:path/filepath"
import "core:strings"
import "core:terminal"
import "core:text/table"
ListEntry :: struct {
Directory: string `json:"directory"`,
@@ -27,8 +28,15 @@ cmd_list :: proc(cmd: ^Command) {
}
if terminal.is_terminal(os.stdout) {
headers := []string{"Directory", "Path"}
table_rows := make([dynamic][]string, 0, len(rows), context.temp_allocator)
t: table.Table
table.init(&t, context.temp_allocator, context.temp_allocator)
table.padding(&t, 1, 1)
table.aligned_header_of_values(
&t,
.Center,
COLOR_TABLE_HEADING + "Directory" + ANSI_RESET,
COLOR_TABLE_HEADING + "Path" + ANSI_RESET,
)
for row in rows {
dir_str := strings.concatenate(
@@ -36,13 +44,11 @@ cmd_list :: proc(cmd: ^Command) {
context.temp_allocator,
)
filename := filepath.base(row.Path)
row_slice := make([]string, 2, context.temp_allocator)
row_slice[0] = dir_str
row_slice[1] = filename
append(&table_rows, row_slice)
table.row(&t, dir_str, filename)
}
render_table(cmd.out, headers, table_rows[:])
table.write_decorated_table(cmd.out, &t, decorations, ansi_aware_width)
} else {
// TODO: Should we instead print full entries here?
entries: [dynamic]ListEntry

View File

@@ -3,8 +3,8 @@ package main
import "core:encoding/json"
import "core:fmt"
import "core:os"
import "core:strings"
import "core:terminal"
import "core:text/table"
SyncEntry :: struct {
Path: string `json:"path"`,
@@ -25,15 +25,7 @@ cmd_sync :: proc(cmd: ^Command) {
return
}
// TODO: Can't use temp allocator becuase strings inside are copied to context.allocator
results := make([]SyncEntry, len(files))
defer {
for &e in results {
delete(e.Path)
delete(e.Status)
}
delete(results)
}
results := make([]SyncEntry, len(files), context.temp_allocator)
for &file, i in files {
result, err := db_sync(&db, &file)
@@ -51,26 +43,29 @@ cmd_sync :: proc(cmd: ^Command) {
status = "OK"
}
// TODO: Handle errors
path_str, _ := strings.clone(file.Path, context.temp_allocator)
status_str, _ := strings.clone(status, context.temp_allocator)
results[i] = SyncEntry {
Path = path_str,
Status = status_str,
Path = file.Path,
Status = status,
}
}
if terminal.is_terminal(os.stdout) {
headers := []string{"File", "Status"}
// TODO: Use [2]string instead of slice here
table_rows := make([dynamic][]string, 0, len(results), context.temp_allocator)
t: table.Table
table.init(&t, context.temp_allocator, context.temp_allocator)
table.padding(&t, 1, 1)
table.aligned_header_of_values(
&t,
.Center,
COLOR_TABLE_HEADING + "File" + ANSI_RESET,
COLOR_TABLE_HEADING + "Status" + ANSI_RESET,
)
for res in results {
row_slice := [2]string{res.Path, res.Status}
append(&table_rows, row_slice[:])
table.row(&t, res.Path, res.Status)
}
render_table(cmd.out, headers, table_rows[:])
table.write_decorated_table(cmd.out, &t, decorations, ansi_aware_width)
} else {
data, marshal_err := json.marshal(results[:], allocator = context.temp_allocator)
if marshal_err != nil {

View File

@@ -11,5 +11,7 @@ COLOR_EXAMPLE :: ansi.CSI + ansi.ITALIC + ansi.SGR
COLOR_FLAGS :: ansi.CSI + ansi.BOLD + ";" + ansi.FG_BRIGHT_WHITE + ansi.SGR
COLOR_TABLE_HEADING :: ansi.CSI + ansi.FG_BRIGHT_GREEN + ansi.SGR
ANSI_RESET :: ansi.CSI + ansi.RESET + ansi.SGR

View File

@@ -1,97 +1,36 @@
package main
import "core:fmt"
import "core:io"
import "core:strings"
import "core:terminal/ansi"
import "core:text/table"
import "core:unicode/utf8"
render_table :: proc(w: io.Writer, headers: []string, rows: [][]string) {
col_widths := make([dynamic]int, 0, len(headers), context.temp_allocator)
for i in 0 ..< len(headers) {
append(&col_widths, strings.rune_count(headers[i]))
}
for r in rows {
for i in 0 ..< len(r) {
rw := strings.rune_count(r[i])
if i < len(col_widths) && rw > col_widths[i] {
col_widths[i] = rw
}
}
}
b: strings.Builder
strings.builder_init(&b, context.temp_allocator)
hline :: proc(
w: io.Writer,
b: ^strings.Builder,
left, mid, right: string,
widths: [dynamic]int,
) {
strings.write_string(b, left)
for i in 0 ..< len(widths) {
for _ in 0 ..< widths[i] + 2 {
strings.write_string(b, "\u2500")
}
if i < len(widths) - 1 {
strings.write_string(b, mid)
} else {
strings.write_string(b, right)
}
}
fmt.wprintf(w, "%s\n", strings.to_string(b^), flush = false)
strings.builder_reset(b)
}
hline(w, &b, "\u250c", "\u252c", "\u2510", col_widths)
cell :: proc(b: ^strings.Builder, s: string, width: int, color: string = "", center := false) {
before: int
after: int
total_pad := width - strings.rune_count(s)
if center {
before = total_pad / 2
after = total_pad - before
} else {
before = 0
after = total_pad
}
fmt.sbprintf(
b,
" %s%s%s%*s%s%*s%s \u2502",
ansi.CSI,
color,
ansi.SGR,
before,
"",
s,
after,
"",
ansi.CSI + ansi.RESET + ansi.SGR,
)
}
strings.write_string(&b, "\u2502")
for i in 0 ..< len(headers) {
cell(&b, headers[i], col_widths[i], ansi.FG_BRIGHT_GREEN, true)
}
fmt.wprintf(w, "%s\n", strings.to_string(b), flush = false)
strings.builder_reset(&b)
hline(w, &b, "\u251c", "\u253c", "\u2524", col_widths)
for r in rows {
strings.write_string(&b, "\u2502")
for i in 0 ..< len(r) {
cell(&b, r[i], col_widths[i])
}
fmt.wprintf(w, "%s\n", strings.to_string(b), flush = false)
strings.builder_reset(&b)
}
hline(w, &b, "\u2514", "\u2534", "\u2518", col_widths)
decorations := table.Decorations {
"┌",
"┬",
"┐",
"├",
"┼",
"┤",
"└",
"┴",
"┘",
"│",
"─",
}
// TODO: Optimize ansi_aware_width
ansi_aware_width :: proc(str: string) -> int {
buf: [4096]byte
pos := 0
i := 0
for i < len(str) {
if i + 1 < len(str) && str[i] == 0x1b && str[i + 1] == '[' {
i += 2
for i < len(str) {c := str[i]; i += 1; if c >= 0x40 && c <= 0x7E {break}}
} else {
buf[pos] = str[i]; pos += 1; i += 1
}
}
_, _, width := utf8.grapheme_count(string(buf[:pos]))
return width
}

View File

@@ -1,142 +1,33 @@
#+test
package main
import "core:fmt"
import "core:strings"
import "core:terminal/ansi"
import "core:testing"
@(test)
test_render_table_normal :: proc(t: ^testing.T) {
b: strings.Builder
strings.builder_init(&b)
defer strings.builder_destroy(&b)
headers := []string{"Name", "Path"}
rows := [][]string{{"foo", "/home/user/.env"}, {"bar", "/home/user/project/.env"}}
w := strings.to_writer(&b)
render_table(w, headers, rows)
output := strings.to_string(b)
g := ansi.CSI + ansi.FG_BRIGHT_GREEN + ansi.SGR
r := ANSI_RESET
n := ansi.CSI + ansi.SGR
expected := fmt.tprintf(
"┌──────┬─────────────────────────┐\n" +
"│ %sName%s │ %s Path %s │\n" +
"├──────┼─────────────────────────┤\n" +
"│ %sfoo %s │ %s/home/user/.env %s │\n" +
"│ %sbar %s │ %s/home/user/project/.env%s │\n" +
"└──────┴─────────────────────────┘\n",
g,
r,
g,
r,
n,
r,
n,
r,
n,
r,
n,
r,
)
testing.expect(
t,
output == expected,
fmt.tprintf(
"table output mismatch\n--- expected ---\n%s\n--- got ---\n%s\n",
expected,
output,
),
)
test_ansi_aware_width_plain_ascii :: proc(t: ^testing.T) {
testing.expect_value(t, ansi_aware_width("hello"), 5)
}
@(test)
test_render_table_empty :: proc(t: ^testing.T) {
b: strings.Builder
strings.builder_init(&b)
defer strings.builder_destroy(&b)
headers := []string{"Name"}
rows: [][]string
w := strings.to_writer(&b)
render_table(w, headers, rows)
output := strings.to_string(b)
g := ansi.CSI + ansi.FG_BRIGHT_GREEN + ansi.SGR
r := ANSI_RESET
expected := fmt.tprintf(
"┌──────┐\n" +
"│ %sName%s │\n" +
"├──────┤\n" +
"└──────┘\n",
g,
r,
)
testing.expect(
t,
output == expected,
fmt.tprintf(
"table output mismatch\n--- expected ---\n%s\n--- got ---\n%s\n",
expected,
output,
),
)
test_ansi_aware_width_empty :: proc(t: ^testing.T) {
testing.expect_value(t, ansi_aware_width(""), 0)
}
@(test)
test_render_table_unicode :: proc(t: ^testing.T) {
b: strings.Builder
strings.builder_init(&b)
defer strings.builder_destroy(&b)
headers := []string{"Status", "Detail"}
rows := [][]string{{"\u2713 Available", "ok"}, {"\u2717 Missing", "fail"}}
w := strings.to_writer(&b)
render_table(w, headers, rows)
output := strings.to_string(b)
g := ansi.CSI + ansi.FG_BRIGHT_GREEN + ansi.SGR
r := ANSI_RESET
n := ansi.CSI + ansi.SGR
expected := fmt.tprintf(
"┌─────────────┬────────┐\n" +
"│ %s Status %s │ %sDetail%s │\n" +
"├─────────────┼────────┤\n" +
"│ %s✓ Available%s │ %sok %s │\n" +
"│ %s✗ Missing %s │ %sfail %s │\n" +
"└─────────────┴────────┘\n",
g,
r,
g,
r,
n,
r,
n,
r,
n,
r,
n,
r,
)
testing.expect(
t,
output == expected,
fmt.tprintf(
"table output mismatch\n--- expected ---\n%s\n--- got ---\n%s\n",
expected,
output,
),
)
test_ansi_aware_width_with_color_codes :: proc(t: ^testing.T) {
colored := COLOR_TABLE_HEADING + "Directory" + ANSI_RESET
testing.expect_value(t, ansi_aware_width(colored), 9)
}
@(test)
test_ansi_aware_width_unicode :: proc(t: ^testing.T) {
testing.expect_value(t, ansi_aware_width("\u2713 Available"), 11)
testing.expect_value(t, ansi_aware_width("\u2717 Missing"), 9)
}
@(test)
test_ansi_aware_width_multiple_escape_sequences :: proc(t: ^testing.T) {
colored := COLOR_TABLE_HEADING + "a" + ANSI_RESET + "b" + COLOR_TABLE_HEADING + "c" + ANSI_RESET
testing.expect_value(t, ansi_aware_width(colored), 3)
}