dataloader icon indicating copy to clipboard operation
dataloader copied to clipboard

Slice of Pointers (?)

Open K4L1Ma opened this issue 3 years ago • 2 comments

I would like to understand the reasoning behind this BatchFunc type especially why is it a Slice of Pointers as it could potentially lead to memory leakages, as the opposite could even perform better* I think it would be better for it to be just a slice

*some reference: https://groups.google.com/g/golang-nuts/c/C2Ir0GI2gEk/m/fO3Zte1sAgAJ

K4L1Ma avatar Jul 12 '22 13:07 K4L1Ma

I had wondered this myself. As that reference link states, if you have to make 10000 requests, then you make 10001 allocations: 1x []*result[V] and 10000x Result[V] objects. I think you can do a work around however and do it in 2 allocations.

backingResult := make([]Result[V], 10000) // allocate the storage
returnedResult := make([]*Result[V], 10000) // allocate the returned result object
for i := range backingResult {
  returnedResult[i] = &backingResult[i]
}
// do get your results and store them.

No one likes changing an existing API so I think BatchFunc needs stay the same. I think an option would be to make:

type CompactBatchFunc[K comparable, V any] func(context.Context, []K]) []Result[V]

func WrapCompactBatch[K comparable, V any](batchFn CompactBatchFunc[K, V]) BatchFunc[K, V]) {
  return func(ctx context.Context, keys []K) []*Result[V]) {
    result := batchFn(ctx, keys)
    returned := make([]*Result[V], len(result))
    for i := range result {
      returned[i] = &result[i]
    }
    return returned
  }
}

I also think one could put interface magic on the BatchFunc and CompactBatchFunc so a user could pass a BatchFunc or CompactBatchFunc to the NewBatchedLoader function without having to manually WrapCompactBatch.

Are you still interested in this? If a maintainer, such as @pavelnikolov likes this idea, I may have some time to work on it.

harrisonmetz avatar Oct 12 '25 17:10 harrisonmetz

I'm interested in any improvement to the codebase - PRs are more than welcome.

pavelnikolov avatar Oct 12 '25 19:10 pavelnikolov