Tweak tool memory use and optimize shared memory when using preload - #16536
Merged
Conversation
mvdbeek
force-pushed
the
tool_mem_tweaks
branch
3 times, most recently
from
August 15, 2023 15:25
2fabca1 to
ba8059e
Compare
This is in the docutils source code:
```
if level >= self.halt_level:
raise SystemMessage(msg, level)
```
and it's not being caught. Might be new in new docutils?
This needs a whole bunch of refactoring.
mvdbeek
force-pushed
the
tool_mem_tweaks
branch
from
August 17, 2023 09:45
ba8059e to
befed84
Compare
mvdbeek
changed the title
mvdbeek
changed the title
Comparing each element of the static options with each incoming element
is very inefficient. Since dictionaries are ordered this can be
simplified to a simple key value insert.
Prior to this change:
```
parse_static_options (/Users/mvandenb/src/galaxy/lib/galaxy/tool_util/parser/xml.py:1207)
function called 7183 times
313317 function calls in 11.318 seconds
Ordered by: cumulative time, internal time, call count
ncalls tottime percall cumtime percall filename:lineno(function)
7183 11.281 0.002 11.318 0.002 xml.py:1207(parse_static_options)
99675 0.029 0.000 0.033 0.000 __init__.py:1004(string_as_bool)
99675 0.004 0.000 0.004 0.000 {method 'lower' of 'str' objects}
99601 0.004 0.000 0.004 0.000 {method 'append' of 'list' objects}
7183 0.000 0.000 0.000 0.000 {method 'disable' of '_lsprof.Profiler' objects}
0 0.000 0.000 profile:0(profiler)
```
after:
```
*** PROFILER RESULTS ***
parse_static_options (/Users/mvandenb/src/galaxy/lib/galaxy/tool_util/parser/xml.py:1207)
function called 7183 times
220899 function calls in 0.260 seconds
Ordered by: cumulative time, internal time, call count
ncalls tottime percall cumtime percall filename:lineno(function)
7183 0.230 0.000 0.259 0.000 xml.py:1207(parse_static_options)
99675 0.026 0.000 0.029 0.000 __init__.py:1004(string_as_bool)
99675 0.004 0.000 0.004 0.000 {method 'lower' of 'str' objects}
7183 0.000 0.000 0.000 0.000 {method 'values' of 'dict' objects}
7183 0.000 0.000 0.000 0.000 {method 'disable' of '_lsprof.Profiler' objects}
0 0.000 0.000 profile:0(profiler)
```
This is most due to openms_idfilter, which has 2928 static options.
functools.cache is new in python 3.9
For details see benoitc/gunicorn#1640 and https://instagram-engineering.com/copy-on-write-friendly-python-garbage-collection-ad6ed5233ddf I think this is the most subtle to test change. I believe this is working. I started a gunicorn instance with 4 workers: ``` GALAXY_CONFIG_FILE="config/galaxy.yml" gunicorn 'galaxy.webapps.galaxy.fast_factory:factory()' -k galaxy.webapps.galaxy.workers.Worker --pythonpath lib --bind=localhost:8080 --config lib/galaxy/web_stack/gunicorn_config.py --preload -w 4 ``` Then i use the following script against that instance ``` import threading import requests def req(): for i in range(10000): requests.get('http://localhost:8080/history/current_history_json') for i in range(10): threading.Thread(target=req).start() ``` I see that the memory consumption increases much more *during* requests without this commit. It eventually decreases again, but I think not to the same baseline level (hard to tell without more elaborate testing). I attribute the higher memory load during requests to the fact that the garbage collection requiring to inspect more objects, taking more time to run and therefor not running as fast? I'm really not sure, I think we should just roll this out and see, it should be fairly obvious from the grafana dashboards.
mvdbeek
force-pushed
the
tool_mem_tweaks
branch
from
August 17, 2023 11:25
afa3f27 to
4a016e4
Compare
Member
|
Really cool fixes! |
mvdbeek
marked this pull request as ready for review
August 17, 2023 17:29
Member
Author
|
I've tested this against tools-iuc. There are some issues with tmpdir mounting, which I can fix with locally diff --git a/lib/galaxy/tool_util/deps/docker_util.py b/lib/galaxy/tool_util/deps/docker_util.py
index 1cb3b8f1d5..2b0d2988ab 100644
--- a/lib/galaxy/tool_util/deps/docker_util.py
+++ b/lib/galaxy/tool_util/deps/docker_util.py
@@ -150,16 +150,16 @@ def build_docker_run_command(
command_parts.append("--rm")
if run_extra_arguments:
command_parts.append(run_extra_arguments)
- if set_user:
- user = set_user
- if set_user == DEFAULT_SET_USER:
- # If future-us is ever in here and fixing this for docker-machine just
- # use cwltool.docker_id - it takes care of this default nicely.
- euid = os.geteuid()
- egid = os.getgid()
-
- user = "%d:%d" % (euid, egid)
- command_parts.extend(["--user", user])
+ # if set_user:
+ # user = set_user
+ # if set_user == DEFAULT_SET_USER:
+ # # If future-us is ever in here and fixing this for docker-machine just
+ # # use cwltool.docker_id - it takes care of this default nicely.
+ # euid = os.geteuid()
+ # egid = os.getgid()
+
+ # user = "%d:%d" % (euid, egid)
+ # command_parts.extend(["--user", user])
full_image = image
if tag:
full_image = f"{full_image}:{tag}"
I was ready to attribute this to OSX (with the 5? different ways you can run docker) , but why would that be a problem for the IUC CI tests ? Unless something has fundamentally changed in docker ? |
nsoranzo
reviewed
Aug 17, 2023
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
Member
Author
|
I re-ran one of the failing tests in https://github.com/mvdbeek/tools-iuc/actions/runs/5900317393/job/16004762060?pr=23 and it passed, I guess at the time of running I didn't include #16533 in the branch. So this should be ready to go. |
Member
|
Thanks for the fixes! I really appreciate the added typing too! |
|
This PR was merged without a "kind/" label, please correct. |
nsoranzo
added
kind/enhancement
kind/refactoring
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first and most effective change is to drop the ElementTree objects after we're done parsing the tool inputs (5e90cfe, 5d9b010).
This means we have to really be sure to have parsed everything. In particular that means we need to parse help and tests (I've limited this to the webapp) and the
change_formattags.We're only partially parsing the tool help, since rst_to_html is relatively slow (~ 30 seconds for 5620 help sections prior to fc8543b, but even then it is 21 seconds, or 13 with the lru_cache). Instead we're lazily converting from the raw help string to rst, where we cache the rst_to_html function.
How to test the changes?
(Select all options that apply)
License