ARROW-9385: [Python] Fix JPype tests and JVM buffer lifetime #7753
Conversation
|
@github-actions crossbow submit test-conda-python-3.8-jpype |
|
Revision: 68b92e9 Submitted crossbow builds: ursa-labs/crossbow @ actions-414
|
|
@github-actions crossbow submit test-conda-python-3.8-jpype |
|
Revision: bcacea5 Submitted crossbow builds: ursa-labs/crossbow @ actions-415
|
|
@xhochy Can you please validate the buffer handling changes? |
|
Well, this should not go in if every buffer creates a Java memory leak, IMHO. |
AFAIK this isn't introducing a new leak :( |
|
Oh, I see... then we can merge this anyway. |
|
Raised https://issues.apache.org/jira/browse/ARROW-9468 to follow up on the memory leak |
|
It just needs to call |
|
I'm trying to add a nanny object with a custom dtor |
|
@wesm Trying it as well... |
|
@github-actions crossbow submit test-conda-python-3.8-jpype |
|
Revision: 28f6500 Submitted crossbow builds: ursa-labs/crossbow @ actions-416
|
|
Rebased. There were some Java changes in-between, hopefully they won't interfere. Edit: of course they do... |
* fix json representation of metadata object * skip non-nullable pyarrow.null object
|
@github-actions crossbow submit test-conda-python-3.8-jpype |
|
Revision: 4752024 Submitted crossbow builds: ursa-labs/crossbow @ actions-417
|
pitrou
changed the title
|
JPype build is green, AppVeyor is green. Will merge :-) |
| An object that keeps a org.apache.arrow.memory.ArrowBuf's underlying | ||
| memory alive. | ||
| """ | ||
| ref_manager = None |
There was a problem hiding this comment.
Hi @pitrou I was trying to understand why you used a static variable here instead of jvm_buf.retain() like before? I'm a little concerned about how this would behave with multiple threads and buffers that have different ReferenceManagers since retain() is called on the static var, but release() is called on the instance var..
There was a problem hiding this comment.
You are misunderstanding how this works. This is not C++, when you write:
self.ref_manager = ref_managerit creates an instance attribute which shadows the class attribute.
There was a problem hiding this comment.
So if you have multiple threads creating jvm_buffers with different backing reference manager instances, isn't it possible that one thread will overwrite the class attribute ref_manager being used in another thread, then both will call ref_manager.retain() using the same ref_manager class attribute?
There was a problem hiding this comment.
Hmm, no. The class attribute ref_manager will always remain None.
There was a problem hiding this comment.
Oh, ok. I think I was confused over having a local var ref_manager too. So why do you need a class attribute? Is it in case of an exception is thrown and the class instance doesn't get assigned?
There was a problem hiding this comment.
Ah, no, it's not needed. It's just a common idiom to write:
class A:
attr = Noneinstead of:
class A:
def __init__(self):
self.attr = NoneThere was a problem hiding this comment.
So, yes, I could have written self.ref_manager = None inside the constructor instead. Sorry for the confusion.
There was a problem hiding this comment.
Ok, no problem, thanks for clearing that up for me.
No description provided.