You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/08/12 23:34:47 UTC
[GitHub] [incubator-mxnet] samskalicky opened a new pull request #20523: [v1.9.x] stop closing opened libs
samskalicky opened a new pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523
## Description ##
stop closing opened libs. fixes #20411 using portions already merged into master/2.0 in #19016.
According to [[1]](https://pubs.opengroup.org/onlinepubs/009695399/functions/dlclose.html)
> The dlclose() function shall inform the system that the object referenced by a handle returned from a previous dlopen() invocation is no longer needed by the application.
> The use of dlclose() reflects a statement of intent on the part of the process, but does not create any requirement upon the implementation, such as removal of the code or symbols referenced by handle.
So calling `dlclose` is just a suggestion to the OS that it could close the library. But the OS may not do anything (ie. and wait until the process exits to clean up any opened handles). Im thinking we might as well just let it be cleaned up anyway rather than calling `dlclose` at all.
As suggested in [[2]](https://stackoverflow.com/a/26131259)
> If you only ever open one library, use it throughout your program, then calling dlclose just before you exit is probably not essential, but if you open a lot of libraries (e.g. using some sort of plugin in a long-running program that can/will use many different plugins, the program may run out of virtual address space if you don't call dlclose.
> (All shared libraries are closed on exit anyway, so leaving it open at exit should not be an issue)
So if a user was loading/unloading a bunch of libraries at runtime they might want to close some they dont need anymore.
However, since MXNet registers components from the library (ie. operators, partitioners, graph passes, etc) and has NO capability to unregister them the references to the loaded library will live at least as long as MXNet (ie. libmxnet.so) does. MXNet already has quite a few components (or its dependencies like TVM/NNVM) that live forever and require the OS to clean them up when the process exits. So its not possible to close libmxnet.so and assume everything is cleaned up. We might as well do the same thing for custom libraries.
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#discussion_r690670104
##########
File path: include/mxnet/c_api.h
##########
@@ -233,10 +233,11 @@ MXNET_DLL const char *MXGetLastError();
/*!
* \brief Load library dynamically
* \param path to the library .so file
- * \param 0 for quiet, 1 for verbose
+ * \param verbose 0 for quiet, 1 for verbose
+ * \param lib handle to opened library
* \return 0 when success, -1 when failure happens.
*/
-MXNET_DLL int MXLoadLib(const char *path, unsigned verbose);
+MXNET_DLL int MXLoadLib(const char *path, unsigned verbose, void** lib);
Review comment:
C API is part of the API to maintain semantic versioning for. Let's not include the API change then.
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-900570488
Thanks for the PR. The destructor change looks good.
What's the benefit of returning handle to opened library, if it's not recommended that users close them? Is there any other use case?
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#discussion_r689995091
##########
File path: include/mxnet/c_api.h
##########
@@ -233,10 +233,11 @@ MXNET_DLL const char *MXGetLastError();
/*!
* \brief Load library dynamically
* \param path to the library .so file
- * \param 0 for quiet, 1 for verbose
+ * \param verbose 0 for quiet, 1 for verbose
+ * \param lib handle to opened library
* \return 0 when success, -1 when failure happens.
*/
-MXNET_DLL int MXLoadLib(const char *path, unsigned verbose);
+MXNET_DLL int MXLoadLib(const char *path, unsigned verbose, void** lib);
Review comment:
Thanks for the review @szha! No this change isnt required, but without it we cannot return a handle to the opened library. Its not required per-se since opening the same library using ctypes or some other API will return the same handle anyway (with linux doing the mapping). So we can remove this change if that is important, either way is fine to me.
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-898041792
Hey @samskalicky , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [windows-cpu, unix-gpu, centos-cpu, edge, windows-gpu, unix-cpu, clang, miscellaneous, centos-gpu, website, sanity]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#discussion_r690670104
##########
File path: include/mxnet/c_api.h
##########
@@ -233,10 +233,11 @@ MXNET_DLL const char *MXGetLastError();
/*!
* \brief Load library dynamically
* \param path to the library .so file
- * \param 0 for quiet, 1 for verbose
+ * \param verbose 0 for quiet, 1 for verbose
+ * \param lib handle to opened library
* \return 0 when success, -1 when failure happens.
*/
-MXNET_DLL int MXLoadLib(const char *path, unsigned verbose);
+MXNET_DLL int MXLoadLib(const char *path, unsigned verbose, void** lib);
Review comment:
C API is part of the API to maintain semantic versioning for. Let's not include the API change then.
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky edited a comment on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-901309819
Thanks @szha @mseth10 ive made the changes to not return the handle. Now this PR is effectively just not closing opened libraries on exit (and some misc comment updates)
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#discussion_r689950052
##########
File path: include/mxnet/c_api.h
##########
@@ -233,10 +233,11 @@ MXNET_DLL const char *MXGetLastError();
/*!
* \brief Load library dynamically
* \param path to the library .so file
- * \param 0 for quiet, 1 for verbose
+ * \param verbose 0 for quiet, 1 for verbose
+ * \param lib handle to opened library
* \return 0 when success, -1 when failure happens.
*/
-MXNET_DLL int MXLoadLib(const char *path, unsigned verbose);
+MXNET_DLL int MXLoadLib(const char *path, unsigned verbose, void** lib);
Review comment:
This is a C API change. Is it required for the fix?
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-901309819
Thanks @szha @mseth10 ive made the changes to not remove the handle. Now this PR is effectively just not closing opened libraries on exit (and some misc comment updates)
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
mseth10 commented on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-900570488
Thanks for the PR. The destructor change looks good.
What's the benefit of returning handle to opened library, if it's not recommended that users close them? Is there any other use case?
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523#issuecomment-898624815
Manual build on Mac runs the gemm_lib example in extensions without segfault
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 merged pull request #20523: [v1.9.x] stop closing opened libs
Posted by GitBox <gi...@apache.org>.
mseth10 merged pull request #20523:
URL: https://github.com/apache/incubator-mxnet/pull/20523
--
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: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org