-
Notifications
You must be signed in to change notification settings - Fork 253
fix: Fix various memory leaks problems #890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a90f43a
8657a82
a0c46ee
c6ec92c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.comet.vector | ||
|
|
||
| import org.apache.arrow.c.ArrowArray | ||
| import org.apache.arrow.c.ArrowSchema | ||
|
|
||
| /** | ||
| * A wrapper class to hold the exported Arrow arrays and schemas. | ||
| * | ||
| * @param batch | ||
| * a list containing number of rows + pairs of memory addresses in the format of (address of | ||
| * Arrow array, address of Arrow schema) | ||
| * @param arrowSchemas | ||
| * the exported Arrow schemas, needs to be deallocated after being moved by the native executor | ||
| * @param arrowArrays | ||
| * the exported Arrow arrays, needs to be deallocated after being moved by the native executor | ||
| */ | ||
| case class ExportedBatch( | ||
| batch: Array[Long], | ||
| arrowSchemas: Array[ArrowSchema], | ||
| arrowArrays: Array[ArrowArray]) { | ||
| def close(): Unit = { | ||
| arrowSchemas.foreach(_.close()) | ||
| arrowArrays.foreach(_.close()) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,43 +44,53 @@ class NativeUtil { | |
| * @param batch | ||
| * the input Comet columnar batch | ||
| * @return | ||
| * a list containing number of rows + pairs of memory addresses in the format of (address of | ||
| * Arrow array, address of Arrow schema) | ||
| * an exported batches object containing an array containing number of rows + pairs of memory | ||
| * addresses in the format of (address of Arrow array, address of Arrow schema) | ||
| */ | ||
| def exportBatch(batch: ColumnarBatch): Array[Long] = { | ||
| def exportBatch(batch: ColumnarBatch): ExportedBatch = { | ||
| val exportedVectors = mutable.ArrayBuffer.empty[Long] | ||
| exportedVectors += batch.numRows() | ||
|
|
||
| // Run checks prior to exporting the batch | ||
| (0 until batch.numCols()).foreach { index => | ||
| val c = batch.column(index) | ||
| if (!c.isInstanceOf[CometVector]) { | ||
| batch.close() | ||
| throw new SparkException( | ||
| "Comet execution only takes Arrow Arrays, but got " + | ||
| s"${c.getClass}") | ||
| } | ||
| } | ||
|
|
||
| val arrowSchemas = mutable.ArrayBuffer.empty[ArrowSchema] | ||
| val arrowArrays = mutable.ArrayBuffer.empty[ArrowArray] | ||
|
|
||
| (0 until batch.numCols()).foreach { index => | ||
| batch.column(index) match { | ||
| case a: CometVector => | ||
| val valueVector = a.getValueVector | ||
|
|
||
| val provider = if (valueVector.getField.getDictionary != null) { | ||
| a.getDictionaryProvider | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| val arrowSchema = ArrowSchema.allocateNew(allocator) | ||
| val arrowArray = ArrowArray.allocateNew(allocator) | ||
| Data.exportVector( | ||
| allocator, | ||
| getFieldVector(valueVector, "export"), | ||
| provider, | ||
| arrowArray, | ||
| arrowSchema) | ||
|
|
||
| exportedVectors += arrowArray.memoryAddress() | ||
| exportedVectors += arrowSchema.memoryAddress() | ||
| case c => | ||
| throw new SparkException( | ||
| "Comet execution only takes Arrow Arrays, but got " + | ||
| s"${c.getClass}") | ||
| val cometVector = batch.column(index).asInstanceOf[CometVector] | ||
| val valueVector = cometVector.getValueVector | ||
|
|
||
| val provider = if (valueVector.getField.getDictionary != null) { | ||
| cometVector.getDictionaryProvider | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| val arrowSchema = ArrowSchema.allocateNew(allocator) | ||
| val arrowArray = ArrowArray.allocateNew(allocator) | ||
| arrowSchemas += arrowSchema | ||
| arrowArrays += arrowArray | ||
| Data.exportVector( | ||
| allocator, | ||
| getFieldVector(valueVector, "export"), | ||
| provider, | ||
| arrowArray, | ||
| arrowSchema) | ||
|
|
||
| exportedVectors += arrowArray.memoryAddress() | ||
| exportedVectors += arrowSchema.memoryAddress() | ||
| } | ||
|
|
||
| exportedVectors.toArray | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can return
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you don't like the restructuring that moves the sanity checks, I can revert it to the original control flow.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense for the reason https://github.com/apache/datafusion-comet/pull/890/files#r1736808325 |
||
| ExportedBatch(exportedVectors.toArray, arrowSchemas.toArray, arrowArrays.toArray) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,12 +215,8 @@ object Utils { | |
| val writer = new ArrowStreamWriter(root, provider, Channels.newChannel(out)) | ||
| writer.start() | ||
| writer.writeBatch() | ||
|
|
||
| root.clear() | ||
| writer.end() | ||
|
|
||
| out.flush() | ||
| out.close() | ||
| writer.close() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Closing the writer will close the dictionary provider. If the dictionary arrays are shared across batches, you will close them and empty later batches. I remember we hit the issue before.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... I need to take further look at this to fully understand if this fix is correct or not.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be correct. The dictionary provider held by the writer contains copied vectors, so closing them does not interfere with the rest parts of the system.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember closing it will cause some errors in CI due the reason I mentioned. Let's see if CI can pass or not.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CI passes on all commits of this PR, it has run 3 rounds with no problem.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, maybe there was some other changes before. Anyway it is good to close the writer without issue. |
||
|
|
||
| if (out.size() > 0) { | ||
| (batch.numRows(), cbbos.toChunkedByteBuffer) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for moving the sanity check for column types prior to the actual construction of the Arrow C data structure. It is tricky to release already constructed FFI data structures before raising the exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, okay.