-
Notifications
You must be signed in to change notification settings - Fork 29
inner .par collection breaks outer ForkJoinTaskSupport on Linux x86 Scala 2.12.5 #283
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
Comments
I came across this when investigating strange behavior when using parallel collections. Using ktegan's sample code, I was able to replicate this on three different configurations: 2 linux and 1 windows. See my results below. This is really weird. Is using nested parallel collections like this not supported? I've made a gist of the test app so you can quickly wget the code and test on your host. CentOS:
Ubuntu:
Windows (from IntelliJ sbt shell):
On the same Windows host, running with whatever version of scala is in my path at the CLI, it works as expected:
|
@SethTisue, what exactly does it mean for this issue to be moved to the Backlog? It seems to me that the behavior observed here completely breaks the user's control of concurrency in their programs and affects the latest two released versions of scala running on the latest version of OpenJDK's 1.8 series. More information: |
“Backlog” issues are issues we won't hold up any particular release over. but any such classification is only provisional, of course I'd assumed this was a longstanding issue, but if it in fact regressed after 2.12.4, then ah, that's different. so I've moved it to the 2.12.7 milestone (but note that doesn't promise a fix, it just means the issue will at least be considered during release planning) is there a post-2.12.4 change that's the likely culprit? |
Both of these commits are related to ForkJoinPool and are around the right time: |
@SethTisue I'm not sure how it is intended to work. I don't see how my commit would impact this but please let me know if it seems to be causing this problem. |
The example code is executing the "outer" tasks on the custom pool, but the "inner" tasks on the global pool. For the old behavior, set the "task support" also for the "inner" collection. There is also a misunderstanding that parallelism equals threads; it's really the target for running threads. JDK 9 has a new constructor for It might be the |
Thanks @som-snytt ! |
Hold on a second. The problem persists whether or not you set the task support for the inner collection (as mentioned with an alternative code snippet at the bottom of the issue). Also I don't understand your answer. If the outer tasks are running in a custom pool, then why does creating or not creating an inner parallel collection mutate the behavior of the outer collection? An immutable parallel collection in Scala should have the same rough concurrency behavior regardless of what code is executing inside a call to map(). This contract is broken (the outer collection appears to be incorrectly mutated) if an inner collection is created inside of one of the threads because all of a sudden the outer parallel collection spawns a many more threads than requested. The atomicInteger in the test is only measuring the number of threads running in parallel for the outer pool, and so the numbers from the atomicInteger shouldn't really depend on the code executing inside the parallel collection, but the results show otherwise. |
Yes, it's a little mind-bending. The idea is that the outer pool will allocate more threads when it thinks a thread is blocked, i.e., active but not running. When the inner task is run on the same pool, the task is forked, otherwise it submits the task to the other pool. I suspect in the case of submitting to the second pool, joining that task is blocking. Note that it spawns threads to keep That's why the counter in the test isn't really counting threads, it's counting tasks. As an implementation detail, those happen to correspond to threads; but it's not counting running threads; imagine those tasks used managed blocking because they're doing I/O, that would explicitly tell the pool to go ahead and grow to keep making progress. Here's my version of the sample code with println added during the Red Sox game. Comment out setting tasksupport on the "inner" collection to see the bad behavior.
|
Another thought: it might be possible to have the I don't know which behavior would be most expected by users. This probably goes to the core intuition of the framework, that you're using ordinary collections methods but they are parallelized without further ado. |
Thanks, I think I understand things a little bit better now. So this behavior of "the outer pool will allocate more threads when it thinks a thread is blocked" was added after Scala 2.12.4? Is there some way to cap the number of simultaneous tasks? Or alternatively cap the number of active (not running) threads? In my case each task is allocating part of a finite resource (such as memory), so I really want to cap is tasks not running threads. |
Sorry, I also just noticed your comment about setting tasksupport. It has to be the same pool for the inner collection. It sounds like you just want the old behavior, so I would just do that. The issue that changed the behavior was where the inner collection has an explicitly set tasksupport, but previously that was ignored. Otherwise you want the Java 9 FJP as in my comment. NB: I'll submit a PR to use tasksupport only if it is explicitly set by user. In case that is the preferred ergonomics. |
@som-snytt Should we perhaps also consider to use |
@som-snytt I also just noticed that there's a possible issue from 2.13.0-M5 and onwards with the following line: https://github.com/scala/scala-parallel-collections/blob/master/core/src/main/scala/scala/collection/parallel/Tasks.scala#L394 So I venture to say that Tasks should look at if the ExecutionContext isInstanceOf ForkJoinPool as well? |
Thanks for the comments and help everyone, they are well appreciated.
|
@viktorklang , for (3) it sounds like we're saying that the new maximumPoolSize parameter in Java9 will limit the number of live threads instead of limiting the number of currently running threads as (I'm gathering) the parallelism parameter does. Thanks! |
@ktegan To put it differently: |
@viktorklang Thanks. The architecture makes me feel like I need a "conspiracy wall". @ktegan The changed behavior is on 2.12.5. The commit page (your link above) shows the tags and also the PR link. |
@som-snytt I see the link, but my question is how does the code in the link apply to the current issue. You said "The issue that changed the behavior was where the inner collection has an explicitly set tasksupport ..." Based on that I thought you were saying that if the inner collection DOES NOT explicitly set its tasksupport then the PR in the link should have no effect. If you agree with that statement, then my question is if your PR only affects cases where the inner collection explicitly sets the task support, why are we seeing behavior changes when the inner collection does not explicitly set the the task support? |
@ktegan The original issue was #284 as linked from the PR. It's the same as your example, except the bug was that the tasksupport was ignored. If tasksupport is not set explicitly, you get a default tasksupport. The documentation is unambiguous:
https://docs.scala-lang.org/overviews/parallel-collections/configuration.html |
Sorry, my question isn't coming across well. Would you expect the PR you did in #284 to affect cases where the inner collection is not setting the tasksupport? |
Yes, the tasksupport is used whether it was set by user or not. That's why I point out that "each par collection is parameterized with a task support"; I don't see anything which says it is pre-empted in some cases. My Nota Bene above is about the case where it is not explicitly set. |
OK, it seems like there is no simple short term solution to this. Until Java 9 comes out we might move back to 2.12.4 so that the target thread count is respected 99% of the time instead of 0% of the time. Thanks for the help! |
@ktegan I like a happy customer. My previous advice to set the same testsupport on both collections is a simple short term solution. That will be 2.12.4 behavior with respect to this issue. |
But if the body of the map calls functions that call other functions that call other functions I have to pass the instance of the outer collection's testsupport through all of those. Basically any function that could be called inside of an parallel collection now has to take an additional testsupport parameter to be able to have the old behavior. |
That is the issue at hand, @ktegan . |
@som-snytt I'm not positive I understand your comment, but I will assume for now you agree with my assessment. Please let me know if I'm mistaken. |
@ktegan thanks for the report and investigation. having this discussion to refer to could be invaluable to anyone else who runs into this. thx @som-snytt and @viktorklang as well |
I first submitted this to stackoverflow and Volodymyr Glushak was not able to recreate the bug on Mac OS or using scastie. I'm hoping that someone else can recreate this issue or figure out what about my current environment might be causing the problem.
I'm using Scala 2.12.5 and Java OpenJDK 1.8.0_161-b14 on a Linux 3.10.0 x86_64 kernel.
I want to make a parallel collection that uses a fixed number of threads. The standard advice for this is to set tasksupport for the parallel collection to use a ForkJoinTaskSupport with a ForkJoinPool with a fixed number of threads. That works fine UNTIL the processing you are doing in your parallel collection itself uses a parallel collection. When this is the case it appears that the limit set for the ForkJoinPool goes away.
A simple test looks something like the following:
And from this in my environment I get the following output, showing that more than numThreads (in the example 10) threads are running simultaneously if we create a parallel collection inside of incrementAndCountThreads().
Also note that using a ForkJoinTaskSupport in the inner collection does not fix the problem. In other words you get the same results if you use the following code for the inner collection:
Am I missing something? If not is there a way to work around this?
Thanks!
The text was updated successfully, but these errors were encountered: