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

Insufficient task support detection mechanism chooses fails to choose ForkJoin even when available/supported #7237

Closed
scabug opened this issue Mar 9, 2013 · 9 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Mar 9, 2013

The check in https://github.com/scala/scala/blob/master/src/library/scala/collection/parallel/package.scala#L44 ...

  private[parallel] def getTaskSupport: TaskSupport =
    if (scala.util.Properties.isJavaAtLeast("1.6")) {
      val vendor = scala.util.Properties.javaVmVendor
      if ((vendor contains "Oracle") || (vendor contains "Sun") || (vendor contains "Apple")) new ForkJoinTaskSupport
      else new ThreadPoolTaskSupport
    } else new ThreadPoolTaskSupport

... is not sufficient to choose the “best” task support library available.

Example case is Avian where, despite the existence of ForkJoin, ThreadPools are used.

One approach could be to check for scala.util.Properties.isJavaAtLeast("1.7")) and enable ForkJoin without further vendor checks, because one can reasonably expect that if a platform declares Java 7 support it actually does.

@scabug
Copy link
Author

@scabug scabug commented Mar 9, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7237?orig=1
Reporter: @soc
Affected Versions: 2.9.3, 2.10.1-RC3, 2.10.1, 2.11.0-M1
Other Milestones: 2.11.0-M3
See #4960, #7236

@scabug
Copy link
Author

@scabug scabug commented Mar 9, 2013

@soc said:
Additionally, it would make sense to add some check to the detection code for a command line argument so that the task support choice can be overridden and therefore all code paths can be tested.
One example of issues caused by not being able to test this code is #7236.

@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2013

@soc said:
Well, if we could figure out why the vendor checks where added in the first place (digging in the history brought up some issue related to IBM's VM, but no actual tests) we could probably simplify the whole decision mechanism a lot.

@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2013

@axel22 said:
This check was copied from the old stdlib actors library.
Always enabling FJP under the following condition:

scala.util.Properties.isJavaAtLeast("1.7"))

looks ok to me. Feel free to open a pull request.

@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2013

@soc said:
I'm currently running the test suite on IBM's J9 version 6SR13, maybe this stuff can be dropped altogether. Otherwise we would have to devise a way to actually test it.

@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2013

@soc said:
IBM J9 also hangs with ThreadPoolTaskSupport.

@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2013

@soc said:
ForkJoinTaskSupport works on Hotspot, Avian and J9, while ThreadPoolTaskSupport causes the test test/files/scalacheck/parallel-collections to reliably hang on all three.

Therefore, always use ForkJoinTaskSupport:

2.11: scala/scala#2249
2.10: scala/scala#2252

@scabug
Copy link
Author

@scabug scabug commented Apr 3, 2013

@soc said:
Merged.
2.10 scala/scala@67b8de7
2.11 scala/scala@29a9c64.

@scabug scabug closed this Apr 3, 2013
@scabug
Copy link
Author

@scabug scabug commented May 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants