Improve autoref proc management#3470
Conversation
- improve exiting speed - properly handle the dangling thread
|
Didn't remove the auto_ref.sh wrapper, in case someone uses it to run autoref manually. :) |
Lmh-java
requested review from
itsarune,
sauravbanna and
williamckha
as code owners
| env = os.environ.copy() | ||
| env["JAVA_HOME"] = "/opt/tbotspython/bin/jdk" | ||
| autoref_cmd = "/opt/tbotspython/autoReferee/bin/autoReferee -a" |
There was a problem hiding this comment.
will this affect the environment variables of the current shell process (aka the shell that's running the bazel run command?). I think that is why I originally made a separate shell script
There was a problem hiding this comment.
No, I don't think so. Since we did os.environ.copy(), the environment vars are copied, so we are only modifying the copied version. Interestingly, didn't find offical Python doc about this, but using this as a reference. python/cpython#120836
| with open(os.devnull, "w") as fp: | ||
| self.tigers_autoref_proc = Popen( | ||
| autoref_cmd.split(" "), stdout=fp, stderr=fp | ||
| autoref_cmd.split(" "), stdout=fp, stderr=fp, env=env |
There was a problem hiding this comment.
I believe you need to do the same on line 239
itsarune
left a comment
itsarune
left a comment
There was a problem hiding this comment.
nice catch! unfortunately this doesn't run correctly. Autoref expects the binary to be called in a specific directory with respect to the binary, so you'd need to set the working directory to /opt/tbotspython/autoReferee/ before launching it.
Whoops, they hard-coded their config filepath. Should be good now, waiting for the CI. |
| with open(os.devnull, "w") as fp: | ||
| self.tigers_autoref_proc = Popen( | ||
| autoref_cmd.split(" "), stdout=fp, stderr=fp | ||
| autoref_cmd.split(" "), |
There was a problem hiding this comment.
split() also works here, since default separator is any whitespace
| autoref_proto_unix_io.register_observer(Referee, self.referee_buffer) | ||
|
|
||
| def __exit__(self, type, value, traceback) -> None: | ||
| self.end_autoref = True |
There was a problem hiding this comment.
For sure, I will take that ticket and implement the above comment along as well (so I don't get dismissed)
4 participants
Description
We've been investigating the performance degradation of thunderscope over time.Running Thunderscope repeatedly causes your laptop to become increasingly slow.
One of the reasons is that we are not properly cleaning up autoref process after the shutdown thunderscope.
For spinning up autoref, we are Popening a shell script (wrapper), which configures JAVA_HOME and run the auto ref bin. However, the shell script wrapper and the actual autoref (java) are two separated processes with separeted pids. Popen only manages and kills the shell script process, but not the actual autoref.
After I run thunderscope and properly shutdown 3 times, I can see 3 java procs running in the background.
This PR fixes the issue by directly popening the autoref binary.
Testing Done
Resolved Issues
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue