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

Preemption #1533

Merged
merged 5 commits into from Dec 21, 2017

Conversation

Projects
None yet
5 participants
@jhjourdan
Copy link
Contributor

jhjourdan commented Dec 15, 2017

Two changes:

  1. As discussed in MPR#7669, use nanosleep as an implementation of yield for better preemption
  2. Make Thread.delay an alias for Unix.sleepf. There is no longer any point for using select here. Moreover, Unix.sleepf supports (theoretically) nanosecond precision.
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 15, 2017

There are CI failures that need to be investigated. Interestingly, the lib-threads/signal.ml test failed on both amd64 and amd64+flambda testing machines. Does the added fairness make the signal heisen-failure reproducible? (cc @xavierleroy who may be delighted to have a fighting chance to understand this lingering testsuite bug)

I asked the CI to re-run one of them, just in case.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 15, 2017

The other failure is on Windows : the test I added for checking that preemption works fails.

It seems like Sleep(0) is not doing what it says. Should I disable the test on windows? How do I do that? Or does someone has an idea how to fix this on Windows?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 15, 2017

Before you disable the Windows test, I think it would be nice to understand what Sleep(0) actually does.

On how to disable the test: see tests/lib-unix/win-stat/Makefile which does exactly the opposite (disable a test on non-Windows machines).

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 15, 2017

And I am not able to reproduce the signal.ml failure....

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Some comments below. I'm really not convinced that the test should go in the test suite, because unfairness is not a bug, just a quality of implementation issue, and many scheduling accidents can cause it to fail, e.g.CI virtual machines that run on heavily loaded hardware.

/* sched_yield() doesn't do what we want in Linux 2.6 and up (PR#2663) */
/* but not doing anything here would actually disable preemption (PR#7669) */
#ifdef HAS_NANOSLEEP
struct timespec t;

This comment has been minimized.

@xavierleroy

xavierleroy Dec 15, 2017

Contributor

This should be #ifdef __linux__ and it should be put above the two comment lines. Initially at least we want to restrict the change of implementation to Linux, where the problem is known. Later you can test MacOS and all the BSDs to see which is better, nanosleep or sched_yield.

let long_list = generate_list 2000000 in
Printf.printf "Long computation result: %d\n%!"
(List.length (List.rev_map sin long_list))

This comment has been minimized.

@xavierleroy

xavierleroy Dec 15, 2017

Contributor

Pardon my French but this test is awful. On a very fast machine (in the future possibly) the long_computation may well terminate before 0.2 s causing unexpected output. On a slow machine or in bytecode the long_computation may take a long time and slow down the test suite uselessly. Please use Sys.time to iterate a medium-size computation until 2 seconds are elapsed.

This comment has been minimized.

@jhjourdan

jhjourdan Dec 15, 2017

Author Contributor

I changed the test. The reason I did not do that in the first place is that I wondered that the system calls in Sys.time would artificially trigger preemption. But since Sys.time does not release the master lock , there is no risk.

Printf.printf "Interract 1\n";
Thread.delay 0.1;
Printf.printf "Interract 2\n"

This comment has been minimized.

@xavierleroy

xavierleroy Dec 15, 2017

Contributor

"Interact", or better "Interaction", not "Interract".

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch from dac8fcb to f6a9930 Dec 15, 2017

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 15, 2017

I granted all @xavierleroy's wishes, except one: I left the test in the test suite (strengthened as you requested, though). The fact that this is an actual bug or not is arguable, but this can cause real bugs in applications containing UIs. So, if we want users to be able to rely on this better behavior, it better be tested.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 16, 2017

Running the current code through CI precheck gives:
image
Some of the Windows machines are offline right now (light blue dots), but the Windows configurations that were tested are OK. Most of the Unix configurations fail (red dot) because of the dreaded lib-threads/signal.ml test.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 19, 2017

We still have to wait for the CI to complete, but I think I fixed the signal.ml test.

What was happening is that the runner of this test did not wait for the process to terminate, and the checker was actually ran before the process finished to write its output.

As for Windows: I find it strange that Inria CI succeeds while AppVeyor fails. Is there a way of looking in detail the results produced by the AppVeyor CI?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 19, 2017

"Precheck" in progress at Inria CI, stay tuned.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 19, 2017

CI results look very good indeed:
image
(Ignore the light blue / light red / light gray entries, these are configurations that were not tested because the CI is a bit clogged at the moment.)

So, the race condition was in the runner shell script, while I was desperately looking for it in the Caml code... Well spotted indeed, and thanks a lot for solving this mystery!

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Looks very good to me. Could be merged as is but I can't help adding one more nitpick, see below. Follow or ignore the suggestion, as you wish.

t.tv_sec = 0;
t.tv_nsec = 1;
nanosleep(&t, &t);
#else

This comment has been minimized.

@xavierleroy

xavierleroy Dec 19, 2017

Contributor

It is nicer to use NULL as second argument of nanosleep since it makes it obvious that you don't need the remaining time to be stored in t.

This comment has been minimized.

@jhjourdan

jhjourdan Dec 19, 2017

Author Contributor

Done.

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch from 9caa530 to d1078e8 Dec 19, 2017

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 20, 2017

I still have no idea why appveyor is failing, but Inria's precheck isn't.

The only reason I can imagine is that the bug only occurs with msvc in 64 bits mode, which was not available with Inria's CI. @xavierleroy, do you think it would be possible to re-enable the ocaml-msvc-64 machine?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Dec 20, 2017

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

The CI backlog cleared up overnight, so I could finish the test on ocaml-msvc-64. It passes just fine.

I have no idea what's going on with Appveyor, and can't even see how to access the test log files. But remember my prediction that weird scheduling by the OS can always cause testpreempt.ml to fail. Could Appveyor have that kind of weird scheduling?

At any rate, I'd love to merge this PR now but cannot because of the risk of breaking all Appveyor testing. So, could you please turn off the testpreempt.ml test, at least under Windows? It's either this or the PR gets stuck until someone debugs the Appveyor behavior.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 20, 2017

I'm just investigating the AppVeyor failure (sorry for not doing so earlier)

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 20, 2017

@xavierleroy, @shindere - you say it's working on the Inria machines. Can you remind me which version of Visual Studio they're running? I've just seen this failure on an Azure host running msvc64 on VS 2015. There are no "Interaction" lines the test output.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

Thanks @dra27 for looking into this. Concerning the Visual Studio version, the ocaml-msvc-64 machine reports the following:

$ type cl
cl is /cygdrive/c/Program Files (x86)/Microsoft Visual Studio 12.0/VC/BIN/amd64/cl

$ cl -?
Microsoft (R) C/C++ Optimizing Compiler Version 18.00.40629 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

Let me know if you need more information.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 20, 2017

Actually, I think the question about Inria CI might be somewhat more subtle - how many CPUs do the test slaves have?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

The plot thickens :-) ocaml-msvc-64 reports two cores, and its overall throughput is consistent with having two cores, but it's a virtual machine provided by a Cloudstack installation, so that's an approximation of the truth. We run two builds&tests in parallel on this machine, one with flambda and the other without. Each build&test is sequential.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

PS. As far as I remember, the other Windows test machines (msvc, mingw, cygwin, 32 and 64) have the same configuration as ocaml-msvc-64: two-core Cloudstack virtual machines running two builds&test in parallel.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 20, 2017

I think I'm seeing this fail on all 4 ports - though I didn't check that run very thoroughly (I will later this evening - I don't like being unsure of these things, in case you hadn't noticed!).

I was wondering if it might be to do with processor affinity - my reading suggests that Sleep(0) can't yield to a thread scheduled on another CPU. So if the main thread and the secondary thread are on different CPUs, then the main thread never yields. I haven't managed to test this in a way I'm truly satisfied with.

What does seem to work very reliably is changing the Sleep(0) to Sleep(1) in st_thread_yield in st_win32.h. That does make a certain amount of sense, since it must force the thread to be suspended. Not sure what the performance implication of doing that every 50ms is, though?

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 20, 2017

OK, there was something up with my msvc32 environment, but the test fails consistently in mingw32, mingw64 and msvc64 on an 8 vCPU Windows Server 2016 instance.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 20, 2017

Thanks, @dra27, for your help investigating this.

What does seem to work very reliably is changing the Sleep(0) to Sleep(1) in st_thread_yield in st_win32.h. That does make a certain amount of sense, since it must force the thread to be suspended. Not sure what the performance implication of doing that every 50ms is, though?

In theory, that would mean a 2% performance loss in throughput, which is quite a bit. A possible way to avoid this issue would be to yield every 100ms (or more).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 21, 2017

I am very afraid that Sleep(1) could end up sleeping one full clock tick. Again, we won't compromise througput in the name of fairness.

An alternative to be considered is SwitchToThread.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 21, 2017

@xavierleroy - SwitchToThread documents the CPU affinity (IIRC) and I already experimented with it yesterday. It behaves the same as Sleep(0).

I have to say that this behaviour doesn't surprise me on Windows - I'd either expect to have to raise the scheduling priority of one thread, or manually insert yields into the main thread.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 21, 2017

I haven't tested this with the Cygwin ports, because they seemed to be broken (possibly my setup), but I think you are right that this test should be disabled at least on native Windows with a clear reference as to why.

@dra27
Copy link
Contributor

dra27 left a comment

I'll test the Cygwin ports properly just before this gets merged.

# However, this does not seem very reliable, so that this test fails
# on some Windows configurations. See GPR #1533.

test "$TOOLCHAIN" != "msvc" -a "$TOOLCHAIN" != "mingw"

This comment has been minimized.

@dra27

dra27 Dec 21, 2017

Contributor

"$UNIX_OR_WIN32 != "win32" would be better.

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch 2 times, most recently from 128451d to 80505c0 Dec 21, 2017

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 21, 2017

Ok, I've just disabled the test on Windows. We just have to wait for the CI to succeed, and then this PR should be ready to merge.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 21, 2017

You've been caught out by my hastily typed comment - there's a double-quote missing! CI won't catch a Cygwin fault - give me a few minutes!

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch from 80505c0 to 20c1eab Dec 21, 2017

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 21, 2017

Cygwin is affected too - so you also want to test for "$SYSTEM" != "cygwin" (NB both Cygwin and Cygwin64 set SYSTEM=cygwin, unlike the native ports which differentiate 32/64 bit).

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch from 20c1eab to 128451d Dec 21, 2017

@jhjourdan jhjourdan force-pushed the jhjourdan:preemption branch from 128451d to 1cdfcfc Dec 21, 2017

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 21, 2017

Ok, I'm testing for:

test "$OS" != "Windows_NT"

This should work on all Windows ports, right?

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 21, 2017

It's a user-controllable variable and as of the latest insider build you can see that in WSL (interesting aside as to whether this test passes on Ubuntu running on Windows!). Ok today, maybe not tomorrow - but then what ever is ok tomorrow! :)

@dra27

dra27 approved these changes Dec 21, 2017

@xavierleroy xavierleroy merged commit bc72cdf into ocaml:trunk Dec 21, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 21, 2017

It's merged! Thanks to all who participated.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 22, 2017

FYI: we're not done with the signal.ml test... Now, sometimes it blocks forever. Commit c08532f makes sure that the main thread terminates eventually, even if no signal is received, but the blocking behavior persists. Either the OCaml process enters an infinite loop after receiving the signal (?), or the wait in the shell script misses the termination of the OCaml process (??).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 22, 2017

And sometimes it is testpreempt.ml that times out... I'm starting to suspect Jenkins CI itself rather than a problem with OCaml's test suite. Stay tuned.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

jhjourdan commented Dec 22, 2017

Would it be possible that this is a deadlock in the signal handling mechanism? This could be fixed by #1128.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 22, 2017

I was able to reproduce the deadlock and attach gdb. Indeed this is related to #1128 in that the Caml code for the signal handler is run while one of the threads holds the lock for the stdout channel, so the signal handling code waits forever for this lock. Until #1128 is ready, I'll fix the test by not doing any I/O from the signal handler.

hhugo added a commit to janestreet/ocaml that referenced this pull request Jan 25, 2018

Improved Thread.yield / thread preemption (ocaml#1533)
* PR#7669 : under Linux, use nanosleep(1) as the implementation of Thread.yield.
This results in a scheduling of threads that is much more fair than before.

* Add a testpreempt.ml test to check that thread preemption is fair.
This is not yet the case for the Windows ports, so disable the test on Windows.

* Use Unix.sleepf as an implementation of Thread.delay instead of Unix.select.
Unix.sleepf can have better resolution than Unix.select.

* Fix signal.ml test
Add "wait" in the runner shell script in order to make sure that the process has properly exited
before its output is compared with the reference output.

hhugo added a commit to janestreet/ocaml that referenced this pull request Feb 7, 2018

Improved Thread.yield / thread preemption (ocaml#1533)
* PR#7669 : under Linux, use nanosleep(1) as the implementation of Thread.yield.
This results in a scheduling of threads that is much more fair than before.

* Add a testpreempt.ml test to check that thread preemption is fair.
This is not yet the case for the Windows ports, so disable the test on Windows.

* Use Unix.sleepf as an implementation of Thread.delay instead of Unix.select.
Unix.sleepf can have better resolution than Unix.select.

* Fix signal.ml test
Add "wait" in the runner shell script in order to make sure that the process has properly exited
before its output is compared with the reference output.

hhugo added a commit to janestreet/ocaml that referenced this pull request Feb 12, 2018

Improved Thread.yield / thread preemption (ocaml#1533)
* PR#7669 : under Linux, use nanosleep(1) as the implementation of Thread.yield.
This results in a scheduling of threads that is much more fair than before.

* Add a testpreempt.ml test to check that thread preemption is fair.
This is not yet the case for the Windows ports, so disable the test on Windows.

* Use Unix.sleepf as an implementation of Thread.delay instead of Unix.select.
Unix.sleepf can have better resolution than Unix.select.

* Fix signal.ml test
Add "wait" in the runner shell script in order to make sure that the process has properly exited
before its output is compared with the reference output.

lpw25 added a commit to janestreet/ocaml that referenced this pull request Feb 23, 2018

Improved Thread.yield / thread preemption (ocaml#1533)
* PR#7669 : under Linux, use nanosleep(1) as the implementation of Thread.yield.
This results in a scheduling of threads that is much more fair than before.

* Add a testpreempt.ml test to check that thread preemption is fair.
This is not yet the case for the Windows ports, so disable the test on Windows.

* Use Unix.sleepf as an implementation of Thread.delay instead of Unix.select.
Unix.sleepf can have better resolution than Unix.select.

* Fix signal.ml test
Add "wait" in the runner shell script in order to make sure that the process has properly exited
before its output is compared with the reference output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.