-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Integrate the stubprocess.py wrapper from Parts #3703
base: master
Are you sure you want to change the base?
Conversation
This is the original version of the 'stubprocess.py' wrapper, taken from the git repo of the Parts project (https://bitbucket.org/sconsparts/parts.git), commit 4f2a0e97e24b410d098df57ddc4247ebfb2aa636 (v0.15.4).
Modified the original version slightly by creating a shadow version of subprocess.Popen that gets wrapped instead. Also added the license.txt (MIT) from the Parts repo and a short header explaining why the made changes are required.
Hey @dirkbaechle, I posted a question to scons-users regarding SCons process spawning performance, and was referred to your PR (see https://pairlist4.pair.net/pipermail/scons-users/2020-June/008287.html). I also wrote a PoC to tackle this issue: it's not as feature complete as Parts, but as far as I can tell, most of these features aren't used by SCons anyway (in I'd like to test it with the same benchmark, could you please publish the generator script here? |
- Added a new command-line option "--stubprocess-wrapper", which will try to wrap the | ||
subprocess.Popen() call when activated. Enabling this option can give a significant speedup | ||
for the build of large projects that consume a lot of memory and create many files. | ||
Small builds however may experience a slow-down, which is why we declared this option | ||
an "experimental feature" for the moment. Please also refer to the corresponding entry | ||
in the MAN page for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that it is specific to Mac and Linux.
<para>Tries to activate the <filename>stubprocess.py</filename> wrapper from the <emphasis>Parts</emphasis> | ||
project for speeding up builds. This wrapper uses pickling of the Actions' arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this confusing because it implied that Parts is also needed. If this truly is now part of SCons, I personally think that it would be more easily understandable if you either (1) dropped mention of the Parts project or (2) mentioned that it does not require Parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add "a contribution from the Parts project"
Is there no way to create a test(s) for this? |
@bdbaddog Hmm, regarding the test: what would be the list of your acceptance criteria, i.e. what exactly would you like to test? (Sorry, I'm doing too much work with requirements in my day job at the moment. ;) ) |
@Jongy Please check out https://bitbucket.org/dirkbaechle/scons_testresults/src/default/scons230_vs_make/ where I uploaded the script |
Thanks @dirkbaechle. I used your script to generate an environment this way:
And these are the results on my Linux box, in a
I ran with Why so many I then capped
and tried again (this time with
Given those results, I think the approach I presented in |
@Jongy If you have a solution that is even faster than the stubprocess.py, I'd say go for it. I'm personally not stuck to stubprocess.py...I simply took what the Parts project had to offer. One of my main goals wasn't to optimize the wrapper itself, but simply to make it work under SCons while modifying it as little as possible. Like this, it would be easier to keep both of those versions in synch, contributing changes and bugfixes back and forth. What I would be interested in, are times of your wrapper with a lower number of files (6000 or even down to 4000 in total). Is it fast enough, such that we get faster build times even for small projects, or at least the same times roughly? Because then we could activate your wrapper as default. Just my 2cents. |
Cool. Later today I'll post those additional results you've requested, and open a new pull request with my branch. I think that also for projects with very small number of files (e.g even < 100) the process spawning approach will be cheaper. |
Perhaps it can be enabled by default... I made it configurable since I think that new, experimental features like this one shouldn't be enabled by default when first added, and definitely should be disable-able. Following are the results for The
I'll submit a PR soon. |
With this PR we integrate the stubprocess.py wrapper from Parts to our project. It's made available as an experimental feature (for now) via the '--stubprocess-wrapper' command line option.
I considered to simply always activate the wrapper, but the following reasons speak against it:
I ran the wrapper against several 'artificial' builds (created with a Perl script by E. Melski) with the following results (all in parallel with '-j 4'):
There are no additional test cases provided with this PR, running the whole testsuite without the '--stubprocess-wrapper' option didn't show any additional fails on my machine. I also verified that the wrapper works fine in "interactive" mode.
For this current implementation of the wrapper, a Python3 floor version of 3.4 is assumed.
After this PR gets integrated, I plan to do a little report on the scaling of the wrapper over the numbers of cores and the number of files to build. I will then work my results into the 'WhySConsIsNotSlow' Wiki page and update it accordingly.
Contributor Checklist:
CHANGES.txt
(and read theREADME.rst
)