From 92faab2706ca68c7ccf140770efb3153041acb60 Mon Sep 17 00:00:00 2001 From: Spencer Brower Date: Fri, 19 Jun 2026 18:26:25 -0400 Subject: [PATCH] refactor: Used the official table package. --- TABLE_IMPROVEMENT_PLAN.md | 258 -------------------------------------- TODOS.md | 2 + cmd_list.odin | 20 +-- cmd_sync.odin | 37 +++--- colors.odin | 2 + table.odin | 123 +++++------------- table_test.odin | 147 +++------------------- 7 files changed, 83 insertions(+), 506 deletions(-) delete mode 100644 TABLE_IMPROVEMENT_PLAN.md diff --git a/TABLE_IMPROVEMENT_PLAN.md b/TABLE_IMPROVEMENT_PLAN.md deleted file mode 100644 index 027d0fd..0000000 --- a/TABLE_IMPROVEMENT_PLAN.md +++ /dev/null @@ -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 \ No newline at end of file diff --git a/TODOS.md b/TODOS.md index dfec123..efd0798 100644 --- a/TODOS.md +++ b/TODOS.md @@ -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. diff --git a/cmd_list.odin b/cmd_list.odin index d719ce9..fe4ce83 100644 --- a/cmd_list.odin +++ b/cmd_list.odin @@ -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 diff --git a/cmd_sync.odin b/cmd_sync.odin index 8239101..50354c8 100644 --- a/cmd_sync.odin +++ b/cmd_sync.odin @@ -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 { diff --git a/colors.odin b/colors.odin index d31c42a..f9ba9db 100644 --- a/colors.odin +++ b/colors.odin @@ -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 diff --git a/table.odin b/table.odin index 2e967c5..7f33b26 100644 --- a/table.odin +++ b/table.odin @@ -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 } diff --git a/table_test.odin b/table_test.odin index 30a0331..4a2a17d 100644 --- a/table_test.odin +++ b/table_test.odin @@ -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) +}