Repository navigation
Conversation
|
There are several issues that a Codex review caught that I've appended below, but at a higher level, I'm not super excited about having a parallel set of that should only be used for expected large result sets. I'd much rather optimize the existing implementations and have a single function users should call even if it meant that we couldn't get quite as great an optimization. Someone who really cares about performance can use This optimization seems to slow down the few row case (not sure where the break point is). Perhaps the optimization could be merged into the existing functions, but it only triggers after N rows. But I don't want to slow down the common 1-20 row case. 🤷♂️ Codex findings for reference: Existing struct scanners regress. Rebuilding structScanPlan for every row adds approximately 272 bytes and two allocations per row. Against the parent commit, the 1,000-row benchmarks were roughly 10–14% slower. Keep the existing scanning path or cache the additional planning work. The map optimization increases allocations. RowToMapPlan allocates a destination slice plus one *any per column, per row. The PR’s 1,000-row benchmark increased from 1.077 MB / 30,478 allocations to 1.246 MB / 40,495 allocations, without a consistent speed improvement. Using row.Values() would avoid those extra destinations. The closure reuse contract needs to be explicit. Reusing a named plan for SELECT a, b followed by SELECT b, a silently swaps the fields. The documentation should explicitly prohibit reuse across different column layouts and concurrent calls, or the implementation should handle those cases. |
|
The current API doesn't permit this sort of thing, so to unify them I'd have to change the type definition of RowToFunc[T] which is a fairly substantial API break. I wish generics could solve this, but T can't be inferred in calls of CollectRows so there is no backwards compatible option. If you're okay with that I'll put it together; it really needs to be an interface[T] instead of a func type. The rest is easy to handle, but only makes sense if we're breaking the API anyway. I don't see the memory allocation increase when I run the benchmark - I see it slightly decrease at all sizes. I'll try to puzzle that out but I might need to know what system configuration you found that with. |
|
As I mentioned before, I would like to avoid expanding the public interface if possible. I haven't looked into it in depth myself, but I used Astra to investigate alternatives and it claims that a similar level of performance gain can be gained without adding new interfaces. I'd want to investigate that thoroughly before adding new public interfaces. Astra's report is attached below. PR 2661 proposes six closure factory functions to speed up struct scanning by retaining a scan plan across rows. The optimization is promising, but the additional public API is not necessary: an internal cache owned by a collection can deliver nearly the same measured benefit through the existing functions. Recommendation: pursue internal caching in Where the cost comes fromThe existing scanners already cache struct field mappings globally. Struct tag discovery and name matching are not repeated in full on every row. The named scanner still constructs a column-name cache key, looks up the mapping, allocates a destination slice, and uses reflection to populate that slice with pointers into a fresh result struct. The positional scanner also looks up its mapping and allocates and populates destinations, but does not construct a column-name key. The PR retains the mapping and destination slice in a closure. It also separates direct and embedded fields to reduce reflection overhead. It still reflects over each new struct to obtain field addresses; it does not eliminate per-row reflection. The underlying opportunity is to retain reusable scanning work for the lifetime of a result set. Requiring callers to create and manage closures is one way to own that state, but it is not essential to the optimization. Proposed internal designKeep the current call unchanged: items, err := pgx.CollectRows(rows, pgx.RowToStructByName[Item])
This does not require changing A production implementation should separate immutable field metadata, shared through the existing global caches, from mutable destination scratch space owned by the collection. Refresh the destination pointers for every new result struct; never share mutable destinations globally or reuse a result object across returned rows. The cache must distinguish destination type and matching mode, including positional, named, and named-lax scans. Callbacks that alternate types or modes must select or rebuild the appropriate entry. Collection ownership removes the need to identify query streams by comparing arbitrary row interfaces, or to expose cross-query reuse rules to callers. Prototype evidenceA temporary prototype added a private collection wrapper and taught the existing named scanner to obtain a cached closure from it. It reused the PR’s plan construction to isolate the effect of moving state ownership behind the existing API. It did not implement the proposed leaner metadata and scratch-space design or all scanning variants. The following measurements use 1,000 fake rows with twelve integer fields and a preallocated result slice. The baseline is the PR’s unplanned scanner path. Two runs used Go 1.27.0 on Linux amd64 with an Intel Core Ultra 9 285K.
The internal cache captured nearly all of the closure approach’s benefit in this microbenchmark. A new public API is therefore not required to obtain this class of speedup. These measurements isolate scanning overhead. Fake rows omit PostgreSQL transport and decoding costs, so the figures do not establish end-to-end database performance or statistical equivalence. Small results and compatibilityThe simple prototype is not ready to ship. For one row, scanning increased from approximately 365 ns on the existing path to 605–717 ns with the internal cache. It inherited the PR’s plan-construction overhead and added wrapper and closure setup. The next implementation should avoid rebuilding direct and nested field metadata for every collection and investigate retaining the destination work already needed for the first row. Whether to delay caching until a later row should be decided by measurement; the current evidence does not establish the right threshold. Two scope decisions remain:
The added collection machinery must also be measured with scalar and custom callbacks that do not use struct scanning. Findings in the current PRTwo focused tests reproduced problems in the PR’s row-identity check at rows.go line 869:
These reproductions concern custom row implementations; they do not demonstrate that native pgx or pgxpool rows reuse the same object across queries. They illustrate the additional correctness obligations created by the public closure reuse contract. The findings were reproduced against PR head Recommended next stepDevelop an internal collection-cache implementation without introducing the six public plan functions. Validate correctness for all existing struct scanner variants, embedded and ignored fields, mismatched columns, alternating callback types and modes, pgxpool rows, custom row implementations, and independence of returned values. Benchmark 1, 2, 10, 20, and large row counts across narrow and wide structs, alongside scalar and custom callbacks. Include allocation measurements and end-to-end PostgreSQL runs. Acceptance should require preserving existing semantics, avoiding a material small-result regression, and retaining most of the large-result improvement. The prototype supports this direction; it does not yet prove those acceptance criteria are met. |
|
Okay, this is a proposal I thought of and rejected already, but I'm happy to put it together if that's what you really want. The reason why I rejected this idea is because it's not extensible at all: the only operators which can use it would be the ones implemented in the pgx package - so this solves for pgx.RowToStructByName but any user-supplied RowToFunc[T] wouldn't be able to take advantage of it and would have to reproduce the thing I did in this PR on their own. I really didn't expect you to be willing to accept that change, because it only fixes my specific problem - but it does fix my problem, and if you want that I'm okay with it. I'll work up a PR for it. |
🤷 I don't know. My goal has been avoid expanding the public interface. pgx is already pretty big. This is especially true for the helpers like |
|
If my specific problem is inspiring, I can describe what I'm dealing with: I've got a bunch of places in code written by a bunch of different teams, where RowToStructByName is exactly the right semantics, but is shockingly cpu hungry when run on millions of rows, to the point where this is the thing sticking out in cpu profiles (in applications which do a bunch of other stuff). I don't need to make it as fast as possible, I just want the big chunk of cpu time back, and ideally in a form which you'll be happy to merge so I don't have to ship a local solution. |
|
Hmm... to be honest I'm not sure what the best approach is. For the project's sake, I'd like to avoid expanding the interface. Also, for your use case, it requires the caller to choose between two functions that do the same thing. Not ideal for the caller either. But as you said, the other optimization path can't be used by external implementations. That's unfortunate as well. I think I would still lean toward the other optimization path. But I would look further into if "can't be used by external implementations" is surmountable. My hunch is that it is. |
329c21b to
99b522d
Compare
asuffield
changed the title
|
A bunch of experimenting later, I've come up with something that is at least an improvement - I don't know if you'll like this iteration, but I think you'll at least agree it's better. It's also significantly faster than the previous iteration by carefully eliminating a number of allocs (it's quite touchy about this, especially in the low-row-count scenarios) and getting creative with how the reflection is applied. The change is longer and more involved than I'd really like, but most of the fiddly stuff is necessary to not make the 1-row and 10-row cases worse. With just the two core changes (RowPlan and structScanPlan) the 1k and 1M cases are "solved", the rest is necessary to avoid regressions for small row counts. |
|
I like the direction this is going, but there are a few correctness issues according to Astra. Not sure if it is resolvable while keeping the performance gains. Found two correctness regressions in latest head 99b522d:
|
Summary
Per the discussion on this PR, this replaces the six
RowTo*Planclosure factory functions with automatic collection-scoped plan caching insideAppendRows/CollectRowsplus a single genericRowPlanhelper for customRowToFuncimplementations:Existing callers using
pgx.RowToStructByName,pgx.RowToStructByNameLax,pgx.RowToStructByPos,pgx.RowToAddrOfStruct*,pgx.RowTo, andpgx.RowToAddrOfwithCollectRowsorAppendRowsget the speedup and allocation reductions automatically without any call-site changes.Design
Pooled
collectRowsWrapper inAppendRows:AppendRowswrapsrowsin an unexportedcollectRowsstruct obtained from async.Pool, retaining a reusabletargets []anyslice and a cached plan slot(planType, planVal)(with a lazy overflowplanMapwhen a single callback uses multiple distinct plan types per row).collectRowsand itstargetsslice avoids wrapper heap allocations and reduces even 1-row struct scan allocations compared tomaster.AppendRowscall eliminates cross-query closure reuse hazards and interface-comparability panics on custom value-typeRowsimplementations.Pre-Bound Scratch Struct (
structScanPlan[T, Mode], Zerounsafe):newStructScanPlanallocates a singlestructScanPlan[T, Mode]containing a scratchdst Tand bindstargets[i]to&p.dst's fields once usingreflect.Value.Field(for top-level fields) orreflect.Value.FieldByIndex(for embedded structs).p.dirty == true),p.dstis zeroed in-place beforerow.Scan(p.targets...)(resetting any slice, map, or pointer fields tonilsopgtypenever aliases prior rows) and returned by value.reflectcalls (reflect.Value.Field,Addr(),(*rtype).ptrTo,Interface()), all per-rowjoinFieldNames+sync.Maplookups, and—for value-returning scanners—the per-row heap escape ofvar value T.RowToAddrOfStruct*delegates toRowToStruct*, returning&valueso each row receives a distinct*Twhile sharing the same reflection-free scan path.Extensible
RowPlanHelper for ExternalRowToFuncImplementations:RowToFunc[T]implementations can callpgx.RowPlan(row, init)to cache per-query column index lookups or scan state for the duration of aCollectRows/AppendRowscall without introducing stateful closures.CollectRows/AppendRows(such as inCollectOneRowor a manualRows.Nextloop),RowPlanfalls back to invokinginit(row)directly.Benchmarks vs
master(Apple M3 Pro, Go 1.26, PostgreSQL 18)1. Pure Scanner Overhead (
BenchmarkCollectRowsOverhead, Fake Rows, 12-Field Struct)master(Time · Bytes · Allocs)ByName/1ByPos/1Scalar/1ByName/10ByPos/10Scalar/10ByName/20ByPos/20Scalar/20ByName/100ByPos/100Scalar/100ByName/1kByPos/1kScalar/1kByName/1MByPos/1MScalar/1M2. Existing
BenchmarkSelectRowsSimple*Suite (PostgreSQL 18)master(Time · Bytes · Allocs)CollectRowsByPos/1_rowsCollectRowsByPos/10_rowsCollectRowsByPos/100_rowsCollectRowsByPos/1000_rowsAppendRowsByPos/1_rowsAppendRowsByPos/10_rowsAppendRowsByPos/100_rowsAppendRowsByPos/1000_rowsCollectRowsByName/1_rowsCollectRowsByName/10_rowsCollectRowsByName/100_rowsCollectRowsByName/1000_rowsAppendRowsByName/1_rowsAppendRowsByName/10_rowsAppendRowsByName/100_rowsAppendRowsByName/1000_rows3. End-to-End PostgreSQL 18 (
BenchmarkAppendRowsToStruct, 12-Column Struct)master(Time · Memory · Allocs)ByName/1ByNameLax/1ByPos/1AddrOfByName/1AddrOfByNameLax/1AddrOfByPos/1ByName/10ByNameLax/10ByPos/10AddrOfByName/10AddrOfByNameLax/10AddrOfByPos/10ByName/20ByNameLax/20ByPos/20AddrOfByName/20AddrOfByNameLax/20AddrOfByPos/20ByName/100ByNameLax/100ByPos/100AddrOfByName/100AddrOfByNameLax/100AddrOfByPos/100ByName/1kByNameLax/1kByPos/1kAddrOfByName/1kAddrOfByNameLax/1kAddrOfByPos/1kByName/1MByNameLax/1MByPos/1MAddrOfByName/1MAddrOfByNameLax/1MAddrOfByPos/1M