kernel: use posix_spawn in iostreams if available #3513
Conversation
fingolfin
added
do not merge
|
Will be happy to test on Windows - let me know when ready. |
|
@alex-konovalov while I plan to address @ChrisJefferson remarks, I think this is ready for testing on Windows right now -- the main questions are: (a) does it work / not crash at all, and (b) does children.tst run fast. If both are the case, it is worthwhile to improve this PR. If not, I might as well discard it (and can avoid the work to polish it). |
|
This does not seem to break anything Linux/macOS, but had no chance to try windows 32 and 64 yet. |
|
Note this would also partially fix #3509, which would be nice -- although IO_fork would still be a problem when called explicitly. |
|
On windows 32 bit testinstall passes in cygwin shell, but then fails when it is performed on a Cygwin-free machine from the Windows command line shell: (link to the build - St Andrews only) |
|
Note that #3507 has been merged. As soon as other issues with this PR will be resolved, you can try to reenable that test on Windows to see if this helps with the performance of the |
|
Unfortunately I currently have no way to debug these cygwin failures, which are somewhat baffling. I used to have an account on a machine supplied by @pedritomelenas but unfortunately that doesn't work anymore (I didn't use it for quite some time, too). So right now I have no good way to debug this; I guess I could try to once again set a Windows VM for this, but that's always quite some hassle and I just don't have the time and energy to deal with that right now. |
|
@fingolfin I think you still have the account working there, write me on slack, probably the name of the machine changed... |
|
What exactly is the problem on windows? I ran this on cygwin32 and 64, and seemed to work well. I still reduced the number of reps down to 10 as even posix_spawn isn't super-fast compared to forking on linux. |
1 similar comment
|
What exactly is the problem on windows? I ran this on cygwin32 and 64, and seemed to work well. I still reduced the number of reps down to 10 as even posix_spawn isn't super-fast compared to forking on linux. |
|
@ChrisJefferson well @alex-konovalov reported test failures in 32 bit cygwin above (you can see the test output there, it involves |
|
Something weird is going on, that test error clearly doesn't line up with this PR -- the lines in question don't call TmpName, they call TmpNameAllArchs (if that's a good name for a function is another question, but that's what current master, and this PR, have in testinstall/kernel/streams.tst) |
fingolfin
force-pushed
the
mh/posix_spawn
branch
from
4dc4674 to
b8b5386
Compare
Codecov Report
@@ Coverage Diff @@
## master #3513 +/- ##
==========================================
+ Coverage 84.55% 85.43% +0.87%
==========================================
Files 698 699 +1
Lines 345122 346817 +1695
==========================================
+ Hits 291805 296286 +4481
+ Misses 53317 50531 -2786
|
This should perform much better than fork() on Windows, and not worse than it on Unix variants.
fingolfin
force-pushed
the
mh/posix_spawn
branch
from
b8b5386 to
6516d3f
Compare
|
I see an updated version of this PR - will test it on Windows again. |
|
The new version seem to be better: the above mentioned errors disappeared. It shows only the diffs from #3332. Regarding the performance of children.tst, part of it is switched off in the test file if it's run on Windows. Either I need to login into the machine and run it manually there (will have to wait then) or you can edit this PR to reenable that test, and then I will rerun it via Jenkins - that will be more convenient for my workflow. |
|
@alex-konovalov so if it breaks nothing, but fixes some things, I am tempted to merge it. But of course it'd be nice to have some more testing first. So perhaps it should only be merged after |
|
Which "some more testing" do you have in mind? |
|
Tested on 64-bit Cygwin; LGTM. Might be nice to have an option for this: Either a configure-time flag, or separate implementations using spawn() vs fork()/exec(), with one or the other as the default (probably spawn()). Not sure if it matters all that much though. |
|
I believe that @DominikBernhardt also intends to test this in Windows, so let’s see whether he reports back soon. I support merging this if it’s the case that it improves something and doesn’t make anything worse, even if it doesn’t fix everything. |
|
Ok. I already tested it on 32/64 bit linux / macos as part of the check I've mentioned above 10 days ago. |
|
Tested in a fresh install on an external drive. No errors detected while testing either. |
|
Thank you @pedritomelenas. If you have an opportunity, you can also test Thank you also @embray for testing this. Note that it's fine to test it from cygwin shell, but for the approximation of a cygwin-free machine at least do like @pedritomelenas and start GAP using |
|
On On In both cases children.tst took a long time. |
|
Now testextra. With and with |
|
Thank you very much, @pedritomelenas ! |
|
With
If I remember well, this was already commented on slack. Probably a break should be more desirable here. |
fingolfin
changed the title
fingolfin
removed
the
do not merge
dimpase
added
release notes: added
This should perform much better than fork() on Windows, and not worse than it on Unix variants.
Actually, to be safe, we probably will want to keep
fork()as default. I mainly madeposix_spawndefault so that we can more easily test it.This needs some more testing, esp. the part which changes the working directory.
In the meantime, I wonder if @alex-konovalov could test it on Windows to see if it helps with the performance of the
children.tstfile discussed in issue #3506