[Kernel C API] Fix inplacement issue in C API #47356
Conversation
|
@penpornk This PR aims at solving two inplacement issue in kernel C API, please help to review, thanks very much.
what we changed:
|
|
@jzhoulon Thank you for the PR! I think @saxenasaurabh owns this part. I'll request his review. @saxenasaurabh Would you mind helping review this PR? Or could you please suggest other reviewers? Thank you very much! |
This commit is intended to fix two inplace issue in kernel C API:
1. RefCountIsOne
tensor.RefCountIsOne() is the condition to decide whether the op can
do inplacement(e.g. forward_input), however, TF_Tensor retrieved through
TF_GetInput(), TF_AllocateOutput() makes the refcount always > 1, in
TF_TensorFromTensor, it will call tensor.CopyFrom() and increment the refcount
2. TF_TensorBitcastFrom()
Currently, this API only modifies TF_Tensor's buf pointer, but the src c++ tensor it
copied from is not modified, that makes the TF_Tensor inconsistent with src c++ tensor,
see the following example, it allocated an output TF_Tensor and modified its buffer through
TF_TensorBitcastFrom(), however, src c++ tensor in Op_KernelContext is not modified.
```c++
// Example:
TF_Tensor* a = TF_AllocateOutput(ctx, 0); // src c++ tensor: x
TF_Tensor* b = TF_AllocateTemp(ctx);
TF_TensorBitcastFrom(b, a); // a->buf_ = b_buf;
// src c++ tensor x's buf is not modified, thus x->buf_ != a->buf_
```
|
cc: @allenlavoie |
|
Thanks for finding and proposing a solution to this. cc: @rjpower |
|
This is mostly just a problem for the C kernel APIs? Making TF_Tensor always a non-owning reference seems pretty drastic (who owns the result of TFE_TensorHandleResolve, and can I feed it into an op attribute which then owns it again?). What is the blocker for saying TF_Tensor is tensorflow::Tensor and just reinterpret_casting between the types at the C API boundaries? The TensorInterface abstraction we do to for TFRT tensors? |
|
@saxenasaurabh @allenlavoie Thanks for the comments, I didn't try to make TF_Tensor always a non-owning reference since the TF_Tensor is also used for python tensor binding. In this PR, if TF_Tensor is get from (TF_GetInput, TF_AllocateOutput), that means TF_Tensor is used as Kernel C API, in this case, TF_Tensor will not increments the reference count of Tensor. In other case(such as TF_Tensor used for python tensor bind, ), the logic is not changed. |
|
@allenlavoie Can you please take a look on the above comment from @jzhoulon. Thanks! |
|
@saxenasaurabh and I had a discussion with the team and we think adding "explicit IncRef/DecRef/Borrow semantics on TF_Tensor" might be more suitable for this. This would need an RFC and will miss TF 2.5. @jzhoulon Would you mind closing this PR for now? We can reopen it later if it becomes relevant. (Although I think that's unlikely given the RFC direction.) |
|
Sure, looking forward the RFC! |
This commit is intended to fix two inplace issue in kernel C API:
RefCountIsOne
tensor.RefCountIsOne() is the condition to decide whether the op can
do inplacement(e.g. forward_input), however, TF_Tensor retrieved through
TF_GetInput(), TF_AllocateOutput() makes the refcount always > 1, in
TF_TensorFromTensor, it will call tensor.CopyFrom() and increment the refcount
TF_TensorBitcastFrom()
Currently, this API only modifies TF_Tensor's buf pointer, but the src c++ tensor it
copied from is not modified, that makes the TF_Tensor inconsistent with src c++ tensor,
see the following example, it allocated an output TF_Tensor and modified its buffer through
TF_TensorBitcastFrom(), however, src c++ tensor in Op_KernelContext is not modified.