Fixing attribute name in the ExecutableImage class and adding an event to BufferInstance #235
Conversation
ajakovljevicTT
requested review from
kmitrovicTT and
mrakitaTT
as code owners
|
||||||||||||||
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
==========================================
- Coverage 77.46% 77.23% -0.23%
==========================================
Files 21 21
Lines 1025 1028 +3
==========================================
Hits 794 794
- Misses 231 234 +3 ☔ View full report in Codecov by Sentry. |
ajakovljevicTT
changed the title
| @@ -131,6 +131,9 @@ void BufferInstance::BindApi(PJRT_Api *api) { | |||
| api->PJRT_Buffer_ReadyEvent = | |||
| +[](PJRT_Buffer_ReadyEvent_Args *args) -> PJRT_Error * { | |||
| DLOG_F(LOG_DEBUG, "BufferInstance::PJRT_Buffer_ReadyEvent"); | |||
| BufferInstance *buffer = BufferInstance::Unwrap(args->buffer); | |||
| buffer->on_ready_event_ = new EventInstance(); | |||
There was a problem hiding this comment.
@ajakovljevicTT Everytime you write a naked new you should have a really good reason. This will cause a memory leak since you are not deleting it in BufferInstance destructor, forgetting to do so is the main reason to avoid using naked new :) Can you please fix this with changes in #220?
There was a problem hiding this comment.
Noted, will fix it in the aforementioned PR
There was a problem hiding this comment.
My bad, I've just found out that PJRT API callers are responsible with destroying events, though we should still avoid using naked new, I will add further comments in #220
My previous merge had an error in the naming of
num_addressable_devicesattribute, so this PR changes that. In addition, the multichip xla execution requires that the BufferInstance has aready_event, even if it now used used, so I also added this in this PR.