gemini-code-assist[bot] commented on code in PR #58:
URL: https://github.com/apache/tvm-ffi/pull/58#discussion_r2380558618
##########
python/tvm_ffi/cython/base.pxi:
##########
@@ -112,7 +112,6 @@ cdef extern from "tvm/ffi/c_api.h":
ctypedef struct TVMFFIObject:
int32_t type_index
- int32_t ref_counter
void (*deleter)(TVMFFIObject* self)
Review Comment:
The Cython definition of `TVMFFIObject` is incorrect and does not match the
C struct definition in `include/tvm/ffi/c_api.h`. This will cause memory layout
mismatches and likely lead to crashes or memory corruption. The definition
should include `combined_ref_count` and use the correct signature for the
`deleter` function pointer.
```
ctypedef struct TVMFFIObject:
uint64_t combined_ref_count
int32_t type_index
uint32_t __padding
void (*deleter)(void* self, int flags)
```
##########
include/tvm/ffi/c_api.h:
##########
@@ -220,19 +220,30 @@ typedef enum {
* \note TVMFFIObject and TVMFFIAny share the common type_index header
*/
typedef struct {
+ // Ref counter goes first to align ABI with most intrusive ptr designs.
+ // It is also likely more efficient as rc operations can be quite common
+ // ABI note: Strong ref counter and weak ref counter can be packed into a
single 64-bit field
+ // Hopefully in future being able to use 64bit atomic that avoids extra
reading of
+ // weak counter during deletion.
Review Comment:
This change breaks the ABI compatibility between `TVMFFIObject` and
`TVMFFIAny` regarding the common `type_index` header. Previously, one could
safely cast a `TVMFFIObject*` to a `TVMFFIAny*` to read the `type_index`. This
is no longer possible as `combined_ref_count` is now the first member. This is
a significant ABI change and should be clearly documented. The comment at line
220, `TVMFFIObject and TVMFFIAny share the common type_index header`, is now
incorrect and should be removed or updated.
##########
docs/concepts/abi_overview.md:
##########
@@ -191,9 +191,10 @@ we adopt a unified object storage format, defined as
follows:
```c++
typedef struct TVMFFIObject {
- int32_t type_index;
+ uint32_t strong_ref_count;
uint32_t weak_ref_count;
- uint64_t strong_ref_count;
+ int32_t type_index;
+ uint32_t __padding;
Review Comment:
The documented structure of `TVMFFIObject` here is inconsistent with the
actual implementation in `include/tvm/ffi/c_api.h`. The implementation now uses
a single `uint64_t combined_ref_count` field for reference counting. Please
update the documentation to reflect this change.
```suggestion
uint64_t combined_ref_count;
int32_t type_index;
uint32_t __padding;
```
##########
docs/concepts/abi_overview.md:
##########
@@ -203,9 +204,9 @@ typedef struct TVMFFIObject {
`TVMFFIObject` defines a common 24-byte intrusive header that all in-memory
objects share:
-- `type_index` helps us identify the type being stored, which is consistent
with `TVMFFIAny.type_index`.
-- `weak_ref_count` stores the weak atomic reference counter of the object.
- `strong_ref_count` stores the strong atomic reference counter of the object.
+- `weak_ref_count` stores the weak atomic reference counter of the object.
+- `type_index` helps us identify the type being stored, which is consistent
with `TVMFFIAny.type_index`.
Review Comment:
The description of `TVMFFIObject` fields is inconsistent with the
implementation, which now uses `combined_ref_count`. Please update this list to
describe `combined_ref_count` and `type_index` correctly.
```suggestion
- `combined_ref_count` stores the combined strong (lower 32 bits) and weak
(upper 32 bits) atomic reference counters of the object.
- `type_index` helps us identify the type being stored.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
