You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pixelherodev (via GitHub)" <gi...@apache.org> on 2025/10/30 03:07:23 UTC
[PR] Batch of small optimizations [arrow-go]
pixelherodev opened a new pull request, #556:
URL: https://github.com/apache/arrow-go/pull/556
This primarily consists of reducing allocations.
e.g. 03be6d3f adds a specific case to TypeEqual for ListView types, which avoids reflect.DeepEqual. l.elem.Equal does not allocate; reflect.DeepEqual does, so this reduces allocations when comparing a ListViewType.
8c7729fc changes from referencing flatbug internal types by reference to by-value, which avoids letting them escape to the heap.
e3406582 switches from reflect.DeepEqual to slices.Equal when comparing the child IDs of a Union type, which is functionally equivalent for slices and, again, does not allocate.
9eba6d2c does similar for an arrowflight cookie test, replacing reflect.DeepEquals with maps.Equal; I did a grep for reflect.DeepEqual and noticed it.
7557d15c is probably the most objectionable; it replaces a *float64 with float64 directly for minSpaceSavings, since the current nil value has identical behavior to zero anyways. It's a tiny cleanup IMO, but doesn't really have any practical value.
2d3593f6 is a bit ugly; it inlines a copy of NewReaderFromMessageReader and a copy of NewMessageReader into NewReader - by implementing the two directly, it's able to avoid allocating two copies of the `config` structure, and to avoid looping over and invoking config functions twice. It's kinda ugly though, and the `// types: make(dictTypeMap)` which is copied over suggests that there's further room for improvement.
I didn't drop the two APIs, as they are exposed to users, but IMO the performance win is probably worth it, and it can be cleaned up later if needed.
0273ffe8 is a small optimization to the allocator: the Go allocator banches on each allocation, but both branches are functionally equivalent, so I just merged the two. I doubt this would have a _major_ impact, since the cost of a branch is negligible compared to the cost of a heap allocation, but performance is often won with a lot of small impacts IMO :)
Lastly, for now at least, f1a6a331 statically allocates 4 bytes per IPC messageReader (I did _not_ touch the ArrowFlight implementation), and uses it on every message read, rather than heap-allocating 4 bytes each time Message is called.
There's more to be done, but this should be a much more practically sized chunk to review, and I can add more later :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470458972
hahahaha and once again the verification succeeded _before_ running `go fmt`. :crossed_fingers: that it stays that way..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3468744267
Very nice! Thanks for this! Just got a linting issue to deal with before I can merge this :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470004422
Moreover, I'm not sure the second run would even free the memory of an individual buffer.
Two GC passes should be sufficient that the pool becomes empty - but if the pool becomes empty _during a GC pass_, I don't think that the objects that are removed will _themselves_ become freed?
It's possible a third GC pass would be sufficient... which would also explain why it sometimes works, as it's possible for a GC pass to begin in between the two calls to runtime.GC?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3469978631
My suspicion is that the test is flaky because of timing: if GC triggers twice after the memory is returned to the pool _before the checked allocator runs_, then the cleanup function will run, the Buffer's finalizer runs and Release()s the memory.
Using the checked allocator here is going to be very timing dependent, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3469989789
...the test runs runtime.GC() manually twice, but AFAIK there is _no guarantee_ that the GC will run the finalizers on the same pass that frees them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3465950292
There's a lot of variance in the run times for the existing benchmarks for me, but the before and after for allocations are consistent:
```bash
# branch 'main'
goos: linux
goarch: amd64
pkg: github.com/apache/arrow-go/v18/arrow/ipc
cpu: AMD Ryzen 9 7900X3D 12-Core Processor
BenchmarkIPC/Writer/codec=plain-24 207901 5824 ns/op 8160 B/op 91 allocs/op
BenchmarkIPC/Reader/codec=plain-24 247482 4812 ns/op 6132 B/op 83 allocs/op
BenchmarkIPC/Writer/codec=zstd-24 3618 332506 ns/op 2338754 B/op 150 allocs/op
BenchmarkIPC/Reader/codec=zstd-24 40870 24626 ns/op 25887 B/op 194 allocs/op
BenchmarkIPC/Writer/codec=lz4-24 112738 10490 ns/op 12864 B/op 134 allocs/op
BenchmarkIPC/Reader/codec=lz4-24 133892 9154 ns/op 10115 B/op 124 allocs/op
# branch 'small-optimizations'
BenchmarkIPC/Writer/codec=plain-24 168954 7469 ns/op 8160 B/op 91 allocs/op
BenchmarkIPC/Reader/codec=plain-24 259921 4970 ns/op 5993 B/op 76 allocs/op
BenchmarkIPC/Writer/codec=zstd-24 3633 327594 ns/op 2338740 B/op 150 allocs/op
BenchmarkIPC/Reader/codec=zstd-24 40442 26658 ns/op 25739 B/op 187 allocs/op
BenchmarkIPC/Writer/codec=lz4-24 113894 13151 ns/op 13098 B/op 134 allocs/op
BenchmarkIPC/Reader/codec=lz4-24 134320 8952 ns/op 9951 B/op 117 allocs/op
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3468878047
Okay, go fmt ed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470421050
> I'm not even sure how that middleware test can even be flaky tbh.
Yea that's the issue, I've looked at it several times and still haven't been able to figure out what is flakey with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3469598835
...so, both Verify jobs passed on the prior run before go fmt. I'm assuming those CI tests are not reliable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470516019
🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #556:
URL: https://github.com/apache/arrow-go/pull/556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3465981758
I ran `go test -bench . -benchmem -benchtime 30s` before and after to highlight the variance better without running the test 20 times.
Before:
```
BenchmarkIPC/Writer/codec=plain-24 6277569 6240 ns/op 8160 B/op 91 allocs/op
BenchmarkIPC/Reader/codec=plain-24 6876433 5247 ns/op 6131 B/op 83 allocs/op
BenchmarkIPC/Writer/codec=zstd-24 114764 324831 ns/op 2338736 B/op 150 allocs/op
BenchmarkIPC/Reader/codec=zstd-24 1310571 28743 ns/op 25869 B/op 194 allocs/op
BenchmarkIPC/Writer/codec=lz4-24 2930214 12186 ns/op 12905 B/op 134 allocs/op
BenchmarkIPC/Reader/codec=lz4-24 3445965 10790 ns/op 10174 B/op 124 allocs/op
```
After:
```
BenchmarkIPC/Writer/codec=plain-24 6302840 5482 ns/op 8160 B/op 91 allocs/op
BenchmarkIPC/Reader/codec=plain-24 7418485 4689 ns/op 5995 B/op 76 allocs/op
BenchmarkIPC/Writer/codec=zstd-24 102850 346718 ns/op 2338731 B/op 150 allocs/op
BenchmarkIPC/Reader/codec=zstd-24 1422286 25704 ns/op 25742 B/op 187 allocs/op
BenchmarkIPC/Writer/codec=lz4-24 2921695 11956 ns/op 12880 B/op 134 allocs/op
BenchmarkIPC/Reader/codec=lz4-24 3545550 10179 ns/op 9986 B/op 117 allocs/op
```
Both numbers are a bit worse like this, but more consistent; readers look to be consistently ~5-10% faster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3469985819
The buffer pool used in that test has
```go
New: func() interface{} {
buf := memory.NewResizableBuffer(suite.mem)
runtime.SetFinalizer(buf, func(obj *memory.Buffer) { obj.Release() })
return buf
}
```
This is not reliable. It depends upon the pool getting GCed _before the test finishes_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470200379
Pushed a small fix to the arrow flight tests; I didn't have `localhost` configured locally, so the test failed with `s.lis == nil; s.lis.Addr()`.
With localhost configured correctly locally, those tests are passing...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3470167473
Yeah, readning through AddFinalizer, it's not actually guaranteed that the finalizers will run at _all_, effectively. They also are explicitly run one-at-a-time, so if there's, say, 100 buffers being cleaned up, they'll be cleaned up in serial while runtime.GC finishes and returns.
I'm not even sure how that middleware test can even be flaky tbh.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Batch of small optimizations [arrow-go]
Posted by "pixelherodev (via GitHub)" <gi...@apache.org>.
pixelherodev commented on PR #556:
URL: https://github.com/apache/arrow-go/pull/556#issuecomment-3469936281
Yeah, the ubuntu one passed on the second run with no changes, and the macos image is failing an entirely different one.
The macOS memory leak looks correct, by the way - in that it's leaking intentionally, because the memory is pooled. When returning the memory to the pool, `buffer.ResizeNoShrink` is used; since shrink=false is specified to resize, Free() is not called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org