Skip to content

feat: Cache column offsets for Resource.Get/Set - #2592

Closed
bbernays wants to merge 2 commits into
mainfrom
performance-column
Closed

feat: Cache column offsets for Resource.Get/Set#2592
bbernays wants to merge 2 commits into
mainfrom
performance-column

Conversation

@bbernays

@bbernays bbernays commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

During investigation of a plugin with >100 columns in a single table, it was identified that every Resource.Set call would iterate through all 100+ columns for each column... This PR introduces the capability to cache column names in a map so that this can be a simple lookup

Summary

Resource.Get and Resource.Set resolved a column name by scanning Table.Columns
linearly. Resolving a row calls Set once per column, so a table with n columns cost
O(n²) name comparisons per row — and each comparison strides over a full Column
struct, so wide tables blow the cache line too. storeCQID and StoreCQClientID each
added two more scans per row.

This adds an optional name -> offset cache on Table:

  • Table.BuildColumnIndex() builds it for a table and its relations.
  • Table.ColumnIndex(name) uses it, falling back to Columns.Index.
  • Scheduler.Sync builds it once per table, where the table tree is final.

The cache is a hint, never a source of truth. Table.Columns is exported and
mutated in plenty of places, so a naive map would silently address the wrong column
after a mutation. ColumnIndex instead validates every hit against Columns before
returning it, so a stale cache costs a scan and nothing else. Correctness does not
depend on BuildColumnIndex ever being called, or on invalidation being exhaustive.
The mutators that shift offsets (AddCqIDs, AddCqClientID, OverwriteOrAddColumn,
Copy) drop the cache anyway, to keep the fast path fast.

Performance

Scheduler sync, 2000 rows, every column set through a resolver (M4 Pro, median of 3).
Benchmark was written to measure this and is not included in the PR:

Columns Before After
25 11.8 ms 7.1 ms (−40%)
100 45.0 ms 19.0 ms (−58%)
300 230 ms 53 ms (−77%)

Narrow tables are unaffected; the crossover against a linear scan is ~25 columns.

Notes for reviewers

  • schema.Table now has an unexported field. Any plugin calling cmp.Diff on a
    Table will panic with cannot handle unexported field until it adds
    cmpopts.IgnoreUnexported(schema.Table{}). TestTablesToAndFromArrow needed exactly
    that change. This is the only downstream-visible break and probably deserves a
    release note.
  • Sync now writes to the tables it is given, so a table tree must not be shared
    between concurrent Sync calls. Tables.FilterDfs already returns copies, so the
    normal plugin path is unaffected. Noted at the call site.
  • Not done: resolveColumn already holds the column offset from its
    range table.Columns loop and throws it away. Using it would mean exporting a
    Resource.SetAtIndex, i.e. new public API, and the cache recovers most of that win
    already. Happy to add it if you'd rather have it.

@bbernays
bbernays requested a review from a team as a code owner September 9, 2026 17:00

@erezrokah erezrokah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the performance impact of this? Do you have benchmarks?

Reading the full body now 👀

@erezrokah

Copy link
Copy Markdown
Member

Reading the full body now 👀

What's the time we're saving per a regular sync? 30ms? More?

@bbernays

bbernays commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Reading the full body now 👀

What's the time we're saving per a regular sync? 30ms? More?

For large plugins like AWS CUR that are not bound by reading from s3, the savings can be significant...

assume 100 million rows total... 130 columns per row...

If it can save 20 ms per 2,000 rows then with this PR we could see savings 16.66 minutes of CPU time

@erezrokah erezrokah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. I would be interested to see the impact of a real sync
  2. I think we can also avoid caching and pass down the index, e.g.
// scheduler/resolvers/resolvers.go
for _, resource := range resources {
    for i := range table.Columns {
        resolveColumn(ctx, tableLogger, m, selector, client, resource, i, table.Columns[i], c, classifier)
    }
}

func resolveColumn(..., index int, column schema.Column, ...) {
    if column.Resolver != nil {
        // unchanged — plugin code calls resource.Set(c.Name, v); index cannot reach it
    } else {
        v := funk.Get(resource.GetItem(), c.ToPascal(column.Name), funk.WithAllowZero())
        if v != nil {
            if err := resource.SetAtIndex(index, column.Name, v); err != nil { handleErr(err) }
        }
    }
}

@bbernays

bbernays commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #2593

@bbernays bbernays closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants