You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "spiridonov (via GitHub)" <gi...@apache.org> on 2026/01/20 21:14:44 UTC
[PR] perf(arrow): Reduce the amount of allocated objects [arrow-go]
spiridonov opened a new pull request, #645:
URL: https://github.com/apache/arrow-go/pull/645
### Rationale for this change
`apache/arrow-go` is used in the new query engine in Grafana Loki and we are actively working on improving its performance. Here are few low hanging fruits that reduce allocations by many gigabytes when used on the hot path.
### What changes are included in this PR?
This PR reduces amount of allocated objects.
* `arrow/datatype_binary.go`: straightforward, those `Layout` objects are allocated on each call currently.
* `arrow/schema.go`: `Schema.Fields()` does not return a clone anymore and `NewSchemaWithEndian()` does not clone inputs anymore. All call sites are func-signature compatible, but should not modify those args/results anymore. In our opinion this is a reasonable trade-off for higher performance. I am happy to discuss other ways to implement that.
### Are these changes tested?
* Tested with the current `apache/arrow-go` unit test suite.
* Tested with `grafana/loki` test suite for the query engine.
### Are there any user-facing changes?
Yes. Explained above.
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "spiridonov (via GitHub)" <gi...@apache.org>.
spiridonov commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3779756042
Most of our patterns is like following:
* Use a local variable `[]arrow.Field`
* Build whatever fields we need here
* Pass it into `NewSchema`
* Produce arrow record with new schema
* Forget the variable and move on
We already build fields for a single time use and do not modify them later. So it feels inefficient to clone them right after `NewSchema` call...
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "spiridonov (via GitHub)" <gi...@apache.org>.
spiridonov commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3785999723
@zeroshade ^
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3780059786
> I find it a bit tricky with iter.Seq[Field]. By itself it does not allocate anything. But calling yield on each iteration effectively allocates a record on the heap because 1/ this fails escape test (a record is passed to another function and can be used after the current closure returns) and 2/ the record has pointers inside. Using iter.Seq[Field] will not allocate a slice, but it will allocate the same amount of records on the heap anyway.
I'm confused here, why would yield allocate an entire record on the heap as opposed to just creating a copy of the `Field` on the stack? For clarify, I'm suggesting using the `slices` package so the function would be like:
```go
func (s *Schema) FieldIter() iter.Seq[Field] {
return slices.Values(s.fields)
}
```
and would then be used like:
```go
for f := range sc.FieldIter() {
// do stuff with f
}
```
So I'm curious what your code path is like such that this would result in allocating the entire record on the heap? At worst it would allocate the Schema object on the heap? Unless I'm missing something?
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #645:
URL: https://github.com/apache/arrow-go/pull/645#discussion_r2710111240
##########
arrow/schema.go:
##########
@@ -170,18 +170,17 @@ func NewSchema(fields []Field, metadata *Metadata) *Schema {
func NewSchemaWithEndian(fields []Field, metadata *Metadata, e endian.Endianness) *Schema {
sc := &Schema{
- fields: make([]Field, 0, len(fields)),
+ fields: fields,
Review Comment:
The only problem I see with this is that currently we explicitly do the copy to ensure that if the caller maintains a reference to the slice they can't modify the fields after calling this.
By just putting the slice in there, then technically a caller could modify the slice afterwards and possibly screw up the schema.
Is copying the fields really causing that much memory allocations and leakage?
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3802830668
Okay, I think we can get the current version of this in for now while we discuss ways to improve the `Fields()` method, does that work @spiridonov ?
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #645:
URL: https://github.com/apache/arrow-go/pull/645#discussion_r2710121574
##########
arrow/schema.go:
##########
@@ -194,11 +193,10 @@ func (sc *Schema) WithEndianness(e endian.Endianness) *Schema {
func (sc *Schema) Endianness() endian.Endianness { return sc.endianness }
func (sc *Schema) IsNativeEndian() bool { return sc.endianness == endian.NativeEndian }
func (sc *Schema) Metadata() Metadata { return sc.meta }
-func (sc *Schema) Fields() []Field {
- fields := make([]Field, len(sc.fields))
- copy(fields, sc.fields)
- return fields
-}
+
+// Fields returns the fields of the schema.
+// The result is not a clone of the schema's fields and therefore should not be modified by the caller.
+func (sc *Schema) Fields() []Field { return sc.fields }
Review Comment:
A while back we explicitly converted this into being a clone operation.
Instead of changing this, can we introduce a function that returns `iter.Seq[Field]`? that way we can avoid the slice copy while still preventing a user from being able to modify the slice.
##########
arrow/schema.go:
##########
@@ -207,11 +205,17 @@ func (sc *Schema) FieldsByName(n string) ([]Field, bool) {
if !ok {
return nil, ok
}
- fields := make([]Field, 0, len(indices))
- for _, v := range indices {
- fields = append(fields, sc.fields[v])
+ if len(indices) == 1 {
+ return sc.fields[indices[0] : indices[0]+1], ok
+ } else if len(indices) > 1 {
+ fields := make([]Field, 0, len(indices))
+ for _, v := range indices {
+ fields = append(fields, sc.fields[v])
+ }
+ return fields, ok
+ } else {
+ return nil, false
}
Review Comment:
```suggestion
}
return nil, false
}
```
##########
arrow/schema.go:
##########
@@ -170,18 +170,17 @@ func NewSchema(fields []Field, metadata *Metadata) *Schema {
func NewSchemaWithEndian(fields []Field, metadata *Metadata, e endian.Endianness) *Schema {
sc := &Schema{
- fields: make([]Field, 0, len(fields)),
+ fields: fields,
index: make(map[string][]int, len(fields)),
endianness: e,
}
if metadata != nil {
- sc.meta = metadata.clone()
+ sc.meta = *metadata
Review Comment:
same as above, since the members of the `Metadata` object are slices (pointer types) just copying the metadata struct like this means that the caller could potentially modify the metadata after constructing the schema. This is less concerning than possibly modifying the fields slice so I'm more inclined to not see this as an issue here.
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3787755358
Super weird. Though I'd expect records on the heap in general because they are interface types.
That said, could you avoid the heap alloc by making it a value receiver instead of a pointer receiver? The schema object is fairly small, so a value receiver would avoid the escape, right?
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "spiridonov (via GitHub)" <gi...@apache.org>.
spiridonov commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3779708779
Thank you for taking a look @zeroshade!
I have changed a few things:
* Added a private `newSchema` constructor, so that there is a way to initialize `Schema` without cloning fields and meta. This is used internally by `WithEndianness()` and `AddField()`. The latter used to effectively allocate fields twice (first inside itself and then by calling `NewSchema`).
* Rolled back my `Fields()` change as a middle ground. So your old behavior remains.
Each Loki query can touch hundreds of streams that are spread over thousands of data objects, which results in thousands of arrow records being processed. Small functions such as `NewSchema` or `Fields` called thousands times turn into gigabytes of allocations quickly.
I find it a bit tricky with `iter.Seq[Field]`. By itself it does not allocate anything. But calling `yield` on each iteration effectively allocates a record on the heap because 1/ this fails escape test (a record is passed to another function and can be used after the current closure returns) and 2/ the record has pointers inside. Using `iter.Seq[Field]` will not allocate a slice, but it will allocate the same amount of records on the heap anyway.
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "spiridonov (via GitHub)" <gi...@apache.org>.
spiridonov commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3780975070
That is a great question! I also was confused in the beginning, because I saw those objects being allocated in a memory profile report. I did some research and I think that it happens _sometimes_ because the compiler is conservative. If it is not 100% sure in the result of escape analysis (the value is indeed passed into another func that passes it into another func) or a structure is too large it falls back to allocate it on the heap instead of stack. I did build a sandbox example https://godbolt.org/z/h19PnWM7T which shows `runtime.newobject` clearly. Compare that with a simpler type https://godbolt.org/z/KMznfPxMx which does not allocate on the heap.
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3780704506
The rest of the changes look good to me!
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "spiridonov (via GitHub)" <gi...@apache.org>.
spiridonov commented on PR #645:
URL: https://github.com/apache/arrow-go/pull/645#issuecomment-3806779319
@zeroshade hey! I tried to play with value and pointer receivers, like you suggested, but have not got any improvements. Let's keep this version for now, we can find another solution for Fields later. Thank you!
--
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] perf(arrow): Reduce the amount of allocated objects [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #645:
URL: https://github.com/apache/arrow-go/pull/645
--
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