Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speed up slow build times for ./configure based projects #555

Merged
merged 14 commits into from
Mar 31, 2023
Merged

speed up slow build times for ./configure based projects #555

merged 14 commits into from
Mar 31, 2023

Conversation

jonas-kaufmann
Copy link
Contributor

@jonas-kaufmann jonas-kaufmann commented Mar 25, 2023

I have been investigating https://github.com/se-sic/VaRA/issues/1003. Concretely, I profiled the import time of the script benchbuild/res/wrapping/run_compiler.py.inc. I found that, at least for the VaRA-Tool-Suite, a substantial amount of time is being spent on importing plugins in the form of other projects and experiments which are not actually being used. So the main idea is to prevent that by overriding the benchbuild configuration through environment variables. Together with some other changes on VaRA's side, I managed to achieve about a 2x performance improvement in my tests.

The other thing that might give a speed improvement in the future is to use importlib.metadata to query the installed benchbuild version instead of pkg_resources. pkg_resources takes almost 100 ms on my machine to import, while importlib.metadata is negligible. 1ae6fdb won't do anything at the moment though because there are at least two third-party packages that also use pkg_resources to set __version__.

In my opinion, the change to benchbuild/res/wrapping/run_compiler.py.inc should be completely transparent to the user and the necessary imports are still made when unpickling the project/compiler. @vulder @boehmseb @simbuerg What do you think about this? Am I missing some detail where this might not work in certain cases?

@jonas-kaufmann
Copy link
Contributor Author

jonas-kaufmann commented Mar 25, 2023

This is the import time profile after all optimizations. As you can see, most of the time on the benchbuild side is spent on importing packages related to the database. I am not too familiar with all the code but to me it seems that this can't really be reduced since the database is a core feature of benchbuild.

image

@simbuerg
Copy link
Member

Thanks for working on this!

@simbuerg
Copy link
Member

I never thought about it for long. But I think the sqlalchemy support can be removed "fairly" easily. The only real deep connection to the rest of benchbuild should be in benchbuild.utils.run.RunInfo and benchbuild.utils.actions.
For a bit of perspective: sqlalchemy.migrate will go as soon as I finally get to unbreaking the github CI, because it blocks me from upgading to a newer sqlalchemy version.

The rest should work as you implemented it. Earlier versions of benchbuild relied on the standard pickle module. This relied on being able to have all necessary imports present in the unpickling python interpreter.

@jonas-kaufmann
Copy link
Contributor Author

Oh, I just saw #549. I tested again with sqlalchemy.migrate removed. This yields another nice speed-up and also means that 1ae6fdb actually has the intended effect.

image

@jonas-kaufmann
Copy link
Contributor Author

I never thought about it for long. But I think the sqlalchemy support can be removed "fairly" easily. The only real deep connection to the rest of benchbuild should be in benchbuild.utils.run.RunInfo and benchbuild.utils.actions. For a bit of perspective: sqlalchemy.migrate will go as soon as I finally get to unbreaking the github CI, because it blocks me from upgading to a newer sqlalchemy version.

I had a look again and it's definitely pretty easy to disable sqlalchemy support as well, thanks for your input. My idea is to add a switch for disabling the database in the benchbuild config. I am going to clean up my code so far and then push that into this PR as well.

Everything necessary should be imported when loading (unpickling) either the project or the compiler.
Before, we always used the sqlite in-memory database to "disable" the
database. Now, there is a proper configuration option for this. The
advantage is that this can lead to significant performance improvements
when benchbuild is invoked often, for example, in ./configure based
projects. These kinds of projects invoke the wrapped compiler often to
determine whether certain feature are available.
… without module name

"pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead."
@jonas-kaufmann
Copy link
Contributor Author

Force pushing really wasn't the call, anyway, now the damage is done.

Adding the configuration option and the respective if guards for db/transaction code was pretty straightforward. Below is also the import time profile after all these optimizations. For the VaRA-Tool-Suite, we are now at a ~4-5x faster invocation time for the wrapped compiler.

image

@simbuerg
Copy link
Member

Ignore CI errors with doc steps. This is the ancient package 'pheasant' being out-of-date for python 3.10.

I will replace it with sphinx in a future PR.

- use C yaml loader
- avoid unnecessary parsing in init_from_env()
@jonas-kaufmann
Copy link
Contributor Author

Ok. There was one last thing that was looking weird in the profile and that was the amount of time spent in benchbuild.settings to parse the config. The reason for that is that yaml.load() is slow because yaml seems to be a complex language to parse. I was able to reduce the time necessary though by switching to the C-based yaml loader and by removing unecessary loads in init_from_env().

This is the current state now. I don't see anything else weirdly taking a lot of time in the profile anymore. If you don't have any more ideas either, we can merge this from my side.

image

the parent class yaml.CSafeLoader is implemented in C
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch coverage: 20.87% and project coverage change: -2.70 ⚠️

Comparison is base (480223c) 52.71% compared to head (5f21147) 50.01%.

❗ Current head 5f21147 differs from pull request most recent head eabd76a. Consider uploading reports for the commit eabd76a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
- Coverage   52.71%   50.01%   -2.70%     
==========================================
  Files         120      120              
  Lines        8265     8305      +40     
  Branches     1038     1063      +25     
==========================================
- Hits         4357     4154     -203     
- Misses       3716     3960     +244     
+ Partials      192      191       -1     
Impacted Files Coverage Δ
benchbuild/project.py 56.20% <0.00%> (-0.55%) ⬇️
benchbuild/settings.py 100.00% <ø> (ø)
benchbuild/utils/actions.py 51.06% <0.00%> (-4.98%) ⬇️
benchbuild/utils/db.py 15.95% <0.00%> (-44.69%) ⬇️
benchbuild/utils/revision_ranges.py 37.59% <0.00%> (-1.34%) ⬇️
benchbuild/utils/run.py 29.31% <0.00%> (-1.29%) ⬇️
benchbuild/extensions/compiler.py 40.00% <25.00%> (+0.52%) ⬆️
benchbuild/extensions/run.py 50.00% <33.33%> (+0.98%) ⬆️
benchbuild/extensions/time.py 35.29% <33.33%> (-0.19%) ⬇️
benchbuild/utils/wrapping.py 23.85% <33.33%> (-1.84%) ⬇️
... and 3 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@simbuerg simbuerg merged commit 63612a6 into PolyJIT:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants