You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/30 20:12:00 UTC
[GitHub] [arrow] ssirovica opened a new issue, #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
ssirovica opened a new issue, #14007:
URL: https://github.com/apache/arrow/issues/14007
Hi all,
I've created a bug on Jira here with more details and a sample program: https://issues.apache.org/jira/browse/ARROW-17573. I wanted to share here so others can more easily come across it should they run into the same issue.
If this is not a bug any pointers for how to properly build arrow records with BinaryTypes, and then write to parquet using the PQArrow package would be appreciated.
Thanks!
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade closed issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade closed issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
URL: https://github.com/apache/arrow/issues/14007
--
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
[GitHub] [arrow] ssirovica commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
ssirovica commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232469822
Awesome, it is a relief to have the problem confirmed!
Looking, I think it makes sense to implement the copy in `ToThrift` as that's only one place and the problem will be present for all types as min max are both `[]byte`, so a reference value.
Code gen'ing the copies for every arrow type Encode() is feasible as well though.
> The answer is that row group statistics are also stored in the file level metadata. This means that the file writer itself ends up holding onto the stats for the row group, and since it writes an entire record as one row group, and the record is just a single contiguous byte array, the file level statistics for each row group ends up forcing it to keep around the memory for every record.
Ah! So if I understand, we will be retaining the min and max for every row group till the end of the file. However, we should be able to drop these statistics once a row group is written as we don't need to keep them around for parquet.
Would it be worth it to explore a way to not keep the RG statistics long term on the file level metadata? This would save us on copy and memory. With that said it's a more complex change. Also, it would only be a problem if there are many RG's, columns and large min max values.
I'm also happy to try and take a stab at this in a PR!
--
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
[GitHub] [arrow] zeroshade commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232197924
@ssirovica I'll see what I can figure out, an attempt is made to share the buffers whereever possible to avoid copying, but they *should* get freed when the compression is run and the original buffers are no longer needed after the call to `rec.Release()`.
If you have any more findings, share away! I'll post back on here if i'm able to determine anything.
--
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
[GitHub] [arrow] ssirovica commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
ssirovica commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232319128
@zeroshade I think I have a break through for the source of the leak after digging through the debugger!
In the MinMax statistics during write of a row group: https://github.com/apache/arrow/blob/master/go/parquet/metadata/statistics_types.gen.go#L1900, keeping track of the max and min row (by holding the pointer to the byte slice) is retaining the memory.
The capacity of the min/max slice are very large (I assume this is the buffer) while the length is small (11 our "HelloWorld!"). As a quick hack using the same sample program I changed the generated code and copied the bytes. Changing the function to:
```
// SetMinMax updates the min and max values only if they are not currently set
// or if argMin is less than the current min / argMax is greater than the current max
func (s *ByteArrayStatistics) SetMinMax(argMin, argMax parquet.ByteArray) {
maybeMinMax := s.cleanStat([2]parquet.ByteArray{argMin, argMax})
if maybeMinMax == nil {
return
}
min := make([]byte, len((*maybeMinMax)[0]))
max := make([]byte, len((*maybeMinMax)[1]))
copy(min, (*maybeMinMax)[0])
copy(max, (*maybeMinMax)[1])
if !s.hasMinMax {
s.hasMinMax = true
s.min = min
s.max = max
} else {
if !s.less(s.min, min) {
s.min = min
}
if s.less(s.max, max) {
s.max = max
}
}
}
```
The sample program then runs without increasing memory I also diff'd the generated parquet on a small number of records and it was the same.
I think what's missing is understanding why the statistics aren't being released on write of a RG and then resolving that over copying. I plan to investigate this thread a bit more. The copy example above was just to see if we're on the right track.
Let me know if this sounds plausible, would appreciate your thoughts :)
--
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
[GitHub] [arrow] zeroshade commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1233039005
> If I understand, we will be retaining the min and max for every row group till the end of the file. However, we can drop these statistics once a row group is written as we don't need to keep them around for parquet.
While you don't *need* to keep them around for Parquet, it's often good to do so if there are mulitple Row Groups in a file. The file level metadata contains information on each row group in the file including where in the file those row groups start and end. By including the row group level statistics in the file-level metadata, a reader that is performing filtering or otherwise could benefit from the statistics can skip entire row groups without having to separately read the row group metadata. That's why this information is included in the file level metadata. Since the file-level metadata isn't written until the very end of the file, it's necessary to keep this information around (albeit in thrift serialized form).
> Also, it would only be a problem if there are many RG's, columns and large min max values.
Typically most query engines actually suggest using only a single row group rather than many row groups in a single file, so I don't think it's particularly worth-while to look into avoiding keeping the RG statistics long term. However, it would definitely make sense to update the ByteArray types to copy when Encoding so as to avoid this memory issue. If you want to file the PR, go for it! otherwise i'd be happy to create the PR to fix this.
--
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
[GitHub] [arrow] zeroshade commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1233071868
@ssirovica given how small it was, I decided to make the PR to fix the stats mem-leak issue myself. I've run your mainprog with it and confirmed it alleviates the memory issue.
--
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
[GitHub] [arrow] zeroshade commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232143165
I'm trying to replicate your issue from that Jira card and I'm having difficulty. Your main seems to only take about 1 - 2 minutes to run to completion and, at least on my laptop, I can't get it to use more than 2GB of virtual memory so I don't see the 16GB you mentioned in the JIRA card.
Also, since you're using the default allocator (which is just Go's allocator) it's also possible that the memory you're building up is from the reallocations of continuously expanding the string builder's buffers (since you're not reserving the memory) and then the Garbage collector hasn't run to clean it up yet depending on your options.
There's a couple other ideas I can think of, but as I can't currently replicate the 16GB of RAM usage you're seeing it's hard to speculate. Can you give me any more information about the machine you're running this on?
--
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
[GitHub] [arrow] ssirovica commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
ssirovica commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232211982
Looking forward to any insights you have!
Here are a couple key notes from my debugging:
1. I believe it's specifically on the write path. If we don't write to the file IE:
```
if i%2000000 == 0 {
// Write row groups out every 2M times
rec := builder.NewRecord()
// w.Write(rec) Comment out write
rec.Release()
}
```
there is no memory leaking.
2. Using a different schema like from `arrow.PrimitiveTypes` doesn't have this issue. Just BinaryTypes.
3. At one point I used the Checked Allocator (https://github.com/apache/arrow/blob/master/go/arrow/memory/checked_allocator.go#L35). And the reference counts after each record release were 0. However, that's at the lowest level so memory could still be held on elsewhere.
PS: I can share all my heap profiles but since you can reproduce I'll let you grab dumps yourself unless you'd like me to attach 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
[GitHub] [arrow] ssirovica commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
ssirovica commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1233161287
No worries.
I just tried on my machine with the sandbox program and things look good. Thanks for the fast response and insights!
--
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
[GitHub] [arrow] ssirovica commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
ssirovica commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232167498
I confirmed your behavior.
Sorry, I set the test loop a little low. 50,000,000 is not enouch to surface the issue. If you bump it up to 5,000,000,000 (100x) as much you should see the memory grow large.
The machine I'm on is an Intel based macbook. My reported memory from top is over 10G about 40seconds into the program running.
> Also, since you're using the default allocator (which is just Go's allocator) it's also possible that the memory you're building up is from the reallocations of continuously expanding the string builder's buffers (since you're not reserving the memory) and then the Garbage collector hasn't run to clean it up yet depending on your options.
I thought maybe it was something related here. So I have tried forcing the garbage collector to run after every right. That doesn't collect the memory. My suspicion is somewhere in the write path the pointer to the buffers are retained so they don't get GC'd.
Happy to share some more findings.
--
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
[GitHub] [arrow] zeroshade commented on issue #14007: [Go] Writing records to parquet with arrow.BinaryTypes.String leaks memory
Posted by GitBox <gi...@apache.org>.
zeroshade commented on issue #14007:
URL: https://github.com/apache/arrow/issues/14007#issuecomment-1232410957
Not only does it sound plausible, but I had just come to the same conclusion myself!
> I think what's missing is understanding why the statistics aren't being released on write of a RG and then resolving that over copying. The copy example above was just to see if we're on the right track.
The answer is that row group statistics are also stored in the file level metadata. This means that the file writer itself ends up holding onto the stats for the row group, and since it writes an entire record as one row group, and the record is just a single contiguous byte array, the file level statistics for each row group ends up forcing it to keep around the memory for every record. If you look at the `ToThrift` method, it's just propagating the slices for the min and max values, so even the encoded thrift
I believe the solution will be to break the reference and actually perform the copy either in the `ToThrift` method or in the `Encode` methods. I'm gonna sleep on it and see if i can figure out what the best approach is as far as when to break the reference and actually copy the min and max values. If you have any thoughts on it, let me know. Otherwise I'll poke at this more in the morning and see what makes the most sense.
This is definitely a tricky little memory bug you've discovered here, thanks for filing 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