You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/04/01 19:43:40 UTC
[GitHub] [tvm] zxybazh opened a new pull request #10874: [BugFix] Fix PopenWorker Memory Leak
zxybazh opened a new pull request #10874:
URL: https://github.com/apache/tvm/pull/10874
During tuning of meta schedule I found a memory leak from PopenWorker. The cause is related to the use of reader and writer inside of the class. I rewrote the Popenworker to create a new reader / writer before usage. There might slightly impact the overhead of tuning but avoids the memory leak.
CC: @tqchen @junrushao1994
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840899725
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -40,22 +40,32 @@ def main():
if len(sys.argv) != 3:
print("Usage: <read_fd> <write_fd>")
return
- if sys.platform == "win32":
- # pylint: disable=import-outside-toplevel
- import msvcrt
-
- reader = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
- writer = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[2]), os.O_BINARY), "wb")
- else:
- reader = os.fdopen(int(sys.argv[1]), "rb")
- writer = os.fdopen(int(sys.argv[2]), "wb")
logging.basicConfig(level=logging.INFO)
lock = threading.Lock()
+ def get_reader():
+ if sys.platform == "win32":
+ # pylint: disable=import-outside-toplevel
+ import msvcrt
+
+ return os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
+ else:
+ return os.fdopen(int(sys.argv[1]), "rb", 0)
Review comment:
Sure, fixed.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 closed pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
junrushao1994 closed pull request #10874:
URL: https://github.com/apache/tvm/pull/10874
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840923808
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -40,22 +40,34 @@ def main():
if len(sys.argv) != 3:
print("Usage: <read_fd> <write_fd>")
return
- if sys.platform == "win32":
- # pylint: disable=import-outside-toplevel
- import msvcrt
-
- reader = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
- writer = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[2]), os.O_BINARY), "wb")
- else:
- reader = os.fdopen(int(sys.argv[1]), "rb")
- writer = os.fdopen(int(sys.argv[2]), "wb")
logging.basicConfig(level=logging.INFO)
lock = threading.Lock()
+ def get_reader():
Review comment:
sorry that I actually do not fully get why fdopen in a subfunction helps. can you elaborate why? (with an example)
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840895825
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
Offline synced with @zxybazh and this fix seems to only re-opens the same file descriptor each time the popenworker is reused. @junrushao1994 Thanks for the clarification!
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840925066
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -40,22 +40,34 @@ def main():
if len(sys.argv) != 3:
print("Usage: <read_fd> <write_fd>")
return
- if sys.platform == "win32":
- # pylint: disable=import-outside-toplevel
- import msvcrt
-
- reader = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
- writer = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[2]), os.O_BINARY), "wb")
- else:
- reader = os.fdopen(int(sys.argv[1]), "rb")
- writer = os.fdopen(int(sys.argv[2]), "wb")
logging.basicConfig(level=logging.INFO)
lock = threading.Lock()
+ def get_reader():
Review comment:
Hi TQ, it does not acutally help that we put `fdopen` into a subfunction. The cause of the memory leak is acutally reusing the same `reader` and `writer`. The change is acutally to create new `reader` and `writer` everytime instead of reusing the same ones to avoid leak.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840882855
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
fdopen means open a file descriptor, not creating one
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
fdopen means opening a file descriptor, not creating one
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840895824
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
Offline synced with @zxybazh and this fix seems to only re-opens the same file descriptor each time the popenworker is reused. @junrushao1994 Thanks for the clarification.
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
Offline synced with @zxybazh and this fix seems to only re-opens the same file descriptor each time the popenworker is reused. @junrushao1994 Thanks for the clarification.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840897401
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -40,22 +40,32 @@ def main():
if len(sys.argv) != 3:
print("Usage: <read_fd> <write_fd>")
return
- if sys.platform == "win32":
- # pylint: disable=import-outside-toplevel
- import msvcrt
-
- reader = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
- writer = os.fdopen(msvcrt.open_osfhandle(int(sys.argv[2]), os.O_BINARY), "wb")
- else:
- reader = os.fdopen(int(sys.argv[1]), "rb")
- writer = os.fdopen(int(sys.argv[2]), "wb")
logging.basicConfig(level=logging.INFO)
lock = threading.Lock()
+ def get_reader():
+ if sys.platform == "win32":
+ # pylint: disable=import-outside-toplevel
+ import msvcrt
+
+ return os.fdopen(msvcrt.open_osfhandle(int(sys.argv[1]), os.O_BINARY), "rb")
+ else:
+ return os.fdopen(int(sys.argv[1]), "rb", 0)
Review comment:
shall we remove the 0 here?
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #10874: [BugFix] Fix PopenWorker Memory Leak
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #10874:
URL: https://github.com/apache/tvm/pull/10874#discussion_r840882085
##########
File path: python/tvm/exec/popen_worker.py
##########
@@ -69,6 +79,7 @@ def _cancel_run(status):
lock.release()
while True:
+ reader = get_reader()
Review comment:
I am a little concerned by the fact that we are creating file descriptor each time in an infinite loop. If this is a memory leak occurred within a subprocess, would this [issue](https://bugs.python.org/issue12650) more relevant?
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org