Skip to content

perf: pack logical-type name lists (allocNames blob)#83

Merged
mlafeldt merged 2 commits into
duckdb:mainfrom
adubovikov:perf/upstream-pr/1-alloc-packed-names
May 18, 2026
Merged

perf: pack logical-type name lists (allocNames blob)#83
mlafeldt merged 2 commits into
duckdb:mainfrom
adubovikov:perf/upstream-pr/1-alloc-packed-names

Conversation

@adubovikov
Copy link
Copy Markdown
Contributor

Summary

  • Replace per-label C.CString in allocNames with one pointer array + one contiguous NUL-terminated blob (duckdb_malloc), and freeNameList to release both.
  • Remove duckdb_go_bindings_free_names from include/duckdb_go_bindings.h (callers free the blob/array only).
  • Update CreateUnionType, CreateStructType, CreateEnumType, hybrid AppenderCreateQuery (column names only; query/table still use CString until a follow-up PR), and Arrow NewArrowSchema.
  • Add TestCreateEnumType_manyPackedNames and BenchmarkCreateEnumType_64Names.

This is PR 1 of 4 splitting the work from PR #82 as requested for easier review. After this merges, the next branches to rebase onto upstream/main and open as separate PRs are:

  • adubovikov:perf/upstream-pr/2-hot-bind-paths — bind/blob/varchar/vector hot paths without pool
  • adubovikov:perf/upstream-pr/3-nul-pool-corewithNULString pool + Open/Query/Prepare paths
  • adubovikov:perf/upstream-pr/4-nul-pool-sweep — remaining CString routes through the pool

Optional harness (after PR 1–4 land or separately): adubovikov:perf/upstream-pr/5-cpu-bench-harness (cpu_bench/).

Test plan

  • CGO_ENABLED=1 go test ./...
  • CGO_ENABLED=1 go test -tags duckdb_arrow ./...

Replace per-label C.CString in allocNames with a pointer array plus a
contiguous NUL-terminated blob; drop duckdb_go_bindings_free_names.
Add enum regression test and BenchmarkCreateEnumType_64Names.
@adubovikov
Copy link
Copy Markdown
Contributor Author

adubovikov commented May 15, 2026

Draft follow-ups in the same split (rebase each onto main after prior PRs merge): #84 #85 #86 - optional harness: #87.

@taniabogatsch taniabogatsch requested a review from mlafeldt May 17, 2026 16:20
Copy link
Copy Markdown
Member

@mlafeldt mlafeldt left a comment

Choose a reason for hiding this comment

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

Thanks, arena-style allocation is the right call.

Just some minor cleanups left before we can merge this.

Comment thread include/duckdb_go_bindings.h Outdated
Comment thread bindings_alloc_bench_test.go Outdated
Comment thread bindings.go Outdated
Comment thread bindings.go Outdated
Comment thread bindings.go Outdated
…per)

- Remove duckdb_go_bindings_set_name (no longer used).
- Always allocNames(columnNames); freeNameList is nil-safe for empty slice.
- Rename loop length var L -> slen; index name slots with uintptr(charSize).
- Remove unused benchMustOpen from PR1-only bench file.
adubovikov added a commit to adubovikov/duckdb-go-bindings that referenced this pull request May 18, 2026
Same allocNames/AppenderCreateQuery/header adjustments as duckdb#83 follow-up.
@adubovikov
Copy link
Copy Markdown
Contributor Author

Pushed 8794e02 addressing review: removed unused duckdb_go_bindings_set_name, dropped redundant if len(columnNames) > 0 before allocNames/freeNameList, renamed Lslen, use uintptr(charSize) for name pointer stride, removed unused benchMustOpen from this PR’s bench file only. go test ./... and -tags duckdb_arrow pass.

@adubovikov
Copy link
Copy Markdown
Contributor Author

Follow-up on review feedback

Thanks for the review - updates are in 8794e02 on perf/upstream-pr/1-alloc-packed-names.

Topic Change
C helper Removed duckdb_go_bindings_set_name; names are filled only from Go via memcpy into the packed blob.
AppenderCreateQuery Always namesAlloc := allocNames(columnNames) + defer freeNameList(namesAlloc) - empty slice is handled inside allocNames, and freeNameList is nil-safe.
Naming / style Loop length variable renamed from L to slen (avoids uppercase-looking short names).
Pointer stride Name slot indexing uses uintptr(charSize) instead of unsafe.Sizeof(uintptr(0)).
Benchmarks Dropped unused benchMustOpen from this PR's bench file only - later PRs in the split still add shared helpers where needed.

Checks: CGO_ENABLED=1 go test ./... and go test -tags duckdb_arrow ./... - green.

Let me know if anything else should be tweaked before merge.

@adubovikov adubovikov requested a review from mlafeldt May 18, 2026 08:22
@mlafeldt mlafeldt merged commit a3b919d into duckdb:main May 18, 2026
13 checks passed
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