Fixed unlinking the shared memory region on non-Android platform #48475
Conversation
|
@miaowang14 could you review this PR? |
abattery
added
the
TFLiteNNAPIDelegate
| // Each call to ASharedMemory_create produces a unique memory space, hence | ||
| // name should not be used to create the shared memory file, otherwise | ||
| // name should not be unique, otherwise |
There was a problem hiding this comment.
"should be unique"?
Also, could you reformat this section?
| #else | ||
| // Each call to ASharedMemory_create produces a unique memory space, hence | ||
| // name should not be used to create the shared memory file, otherwise | ||
| // two calls to create memory regions using the same 'name', will collide. |
There was a problem hiding this comment.
maybe just simplify these comments to something like "Find a unique file name"?
There was a problem hiding this comment.
Agree, what about this one:
// For non-Android platforms ASharedMemory_create needs unique name to
// create a shared memory object (see nnapi_implementation.cc).
robert-kalmar
force-pushed
the
shmem-unlink-for-non-adroid
branch
from
8263754
to
b487bc2
Compare
google-ml-butler
bot
added
kokoro:force-run
| // create a shared memory object (see nnapi_implementation.cc). | ||
| char shm_name_buffer[L_tmpnam]; | ||
| if (tmpnam(shm_name_buffer) == nullptr) { | ||
| throw new std::runtime_error("NN Memory failed"); |
There was a problem hiding this comment.
The exception here is causing problems and blocking the merge.
Could you remove the exception?
There was a problem hiding this comment.
The exception is removed.
|
@robert-kalmar Can you please check @miaowang14's comments and keep us posted ? Thanks! |
|
@robert-kalmar Any update on this PR? Please. Thanks! |
robert-kalmar
force-pushed
the
shmem-unlink-for-non-adroid
branch
from
b487bc2
to
8332b20
Compare
robert-kalmar
force-pushed
the
shmem-unlink-for-non-adroid
branch
from
8332b20
to
f59f75e
Compare
gbaned
added
ready to pull
| @@ -185,6 +185,9 @@ class NNMemory { | |||
| size_t byte_size_ = 0; | |||
| uint8_t* data_ptr_ = nullptr; | |||
| ANeuralNetworksMemory* nn_memory_handle_ = nullptr; | |||
| #ifndef __ANDROID__ | |||
| std::string shm_region_name; | |||
There was a problem hiding this comment.
hmm, ClangTidy is complaining about the class field variable name, which blocks auto-merger.
Could you change it to "shm_region_name_" ?
There was a problem hiding this comment.
Thanks, I renamed the variable.
robert-kalmar
force-pushed
the
shmem-unlink-for-non-adroid
branch
from
f59f75e
to
be48e0c
Compare
google-ml-butler
bot
added
kokoro:force-run
Current implementation does not unlink the shared memory region for non-Android platforms.