SI-6998 Improvements to the task-support API #1937

soc · 2013-01-20T15:12:32Z

No description provided.

soc · 2013-01-20T15:30:21Z

I'm trying to get the debate started again here: https://groups.google.com/d/topic/scala-internals/lzxVlexkqEg/discussion

Everyone involved (@axel22, @jsuereth, @retronym, ...) in the discussion of #930, could you comment whether this is going into the right direction?

scala-jenkins · 2013-01-20T18:43:53Z

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1460/

scala-jenkins · 2013-01-20T18:45:53Z

jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1460/

scala-jenkins · 2013-01-20T18:56:08Z

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2184/

scala-jenkins · 2013-01-20T18:58:09Z

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2184/

heathermiller · 2013-01-21T17:46:21Z

Given that both @axel22 and @phaller are the parallelism/library guys, I would suggest that @phaller also provide a review on this one.

heathermiller · 2013-01-21T17:56:25Z

src/library/scala/collection/parallel/package.scala

@@ -50,9 +50,10 @@ package object parallel {

  val defaultTaskSupport: TaskSupport = getTaskSupport

-  def setTaskSupport[Coll](c: Coll, t: TaskSupport): Coll = {
+  def setTaskSupport[Coll](c: Coll, taskSupport: TaskSupport): Coll = {


Renaming parameters breaks binary compatibility. It this is necessary (though it seems like just a matter of personal preference to me), you've got to use @deprecatedName. For example...

def setTaskSupport[Coll](c: Coll, @deprecatedName('t) taskSupport: TaskSupport): Coll = { ...

It does break compatibility, albeit of the source kind. Not that that is
any better.

On Monday, January 21, 2013, Heather Miller wrote:

In src/library/scala/collection/parallel/package.scala:

@@ -50,9 +50,10 @@ package object parallel {

val defaultTaskSupport: TaskSupport = getTaskSupport

def setTaskSupport[Coll](c: Coll, t: TaskSupport): Coll = {

def setTaskSupport[Coll](c: Coll, taskSupport: TaskSupport): Coll = {

Renaming parameters breaks binary compatibility. It this is necessary
(though it seems like just a matter of personal preference to me), you've
got to use @deprecatedName. For example...

def setTaskSupport[Coll](c: Coll, @deprecatedName('t) taskSupport: TaskSupport): Coll = { ...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1937/files#r2715869.

That is actually worse for 2.10 -> 2.11, usually we try to maintain as much source compat as possible.

soc · 2013-01-21T22:11:11Z

I'd like to get rid of the setTaskSupport method completely ... would it be acceptable to pass the taskSupport value in the constructor instead?

phaller · 2013-01-25T11:09:14Z

src/library/scala/collection/Parallelizable.scala

@@ -9,6 +9,8 @@
 package scala.collection

 import parallel.Combiner
+import parallel.TaskSupport
+import parallel.setTaskSupport


Better: import parallel.{Combiner, TaskSupport, setTaskSupport}

soc · 2013-01-25T14:15:39Z

@phaller
Thanks a lot!

scala-jenkins · 2013-01-25T15:11:47Z

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2352/

scala-jenkins · 2013-01-25T15:11:48Z

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1621/

paulp · 2013-01-25T15:34:46Z

src/library/scala/collection/parallel/package.scala

@@ -50,9 +51,10 @@ package object parallel {

  val defaultTaskSupport: TaskSupport = getTaskSupport

-  def setTaskSupport[Coll](c: Coll, t: TaskSupport): Coll = {
+  def setTaskSupport[Coll](c: Coll, @deprecatedName('t) taskSupport: TaskSupport): Coll = {


Oh good, you found @deprecatedName.

scala-jenkins · 2013-01-25T15:54:50Z

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2352/

scala-jenkins · 2013-01-25T16:04:49Z

jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1621/

axel22 · 2013-01-28T10:09:19Z

Other than the comment I made on the mailing list about adding new methods to collections, these changes look good to me.

JamesIry · 2013-02-01T16:55:19Z

@axel22 can you link to the mailing list comments, or summarize here?

@phaller have all your issues been addressed?

@retronym, @jsuereth you guys are listed as reviewers, do you feel a need to weight in?

phaller · 2013-02-01T17:01:34Z

src/library/scala/collection/mutable/HashMap.scala

@@ -56,6 +58,9 @@ extends AbstractMap[A, B]

  override def par = new ParHashMap[A, B](hashTableContents)

+  override def parWith(implicit taskSupport: TaskSupport) =
+    setTaskSupport(new ParHashMap[A, B](hashTableContents), taskSupport)


You're still repeating yourself here: the expression new ParHashMap[A, B](hashTableContents) is copied verbatim. Why? Method par already does that.

phaller · 2013-02-01T17:26:37Z

Except for my small comment about DRY, e.g., in mutable.HashMap (sorry for being so persistent), it looks promising to me. Now, if the parameter of parWith is implicit I don't understand why we couldn't fold that into par. (We're allowed to break binary compatibility in 2.11.)

soc · 2013-02-01T19:19:03Z

It will break source compatibility and it will be worse than Java, which manages to handle this task without configuration.

But I will reconsider all solutions and come up with a new proposal, considering that parWith is a terrible name if the argument is implicit.

soc · 2013-02-01T19:27:27Z

@jsuereth Mailing list is https://groups.google.com/d/topic/scala-internals/lzxVlexkqEg/discussion

Current issues (https://groups.google.com/d/msg/scala-internals/lzxVlexkqEg/qLnBWPOy26cJ):

I think that in principle setting those through constructor calls should work. It might require adding the task support parameter to newCombiner, though, or introduce an overload of newCombiner.

Yes, that's exactly what I'm thinking about ... what is the sane approach/default here?

newCombiner + newCombinerWith(implicit taskSupport)
newCombiner + newCombinerWith(taskSupport)
newCombiner(implicit taskSupport)
newCombiner(taskSupport)
newCombiner(taskSupport = defaultTS)

would be the same way as we do for par/parWith but I'm a bit concerned that people don't realize that their implicit taskSupport is used for more stuff than just par/parWith
would require setting it explicitly
would add an implicit argument to the existing method, will break binary compatibility
would add an argument to the existing method, will break source and binary compatibility
would add an default argument to the existing method, will break binary compatibility and would not be in line with the approach taken with par/parWith

The same questions apply to the constructor case, but with the added issue that I can't add an constructor with a different name.

new ParColl(coll, taskSupport)
new ParColl(coll, implicit taskSupport)
new ParColl(coll)(implicit taskSupport)
new ParColl(coll, taskSupport = defaultTS)
new ParColl(coll) + new ParColl(coll, taskSupport)
...

I just realized an additional issue with the “let's add with”-approach when implicit/default methods are used: If the argument is not given explicitly, parWith and friends look horrible!

someCollection.parWith.map(...).filter(...)

Would love to hear your opinions on it!

So this will still need a few revision ... I really want to get this right.

JamesIry · 2013-02-06T21:47:57Z

Should we close this PR and hash out the design on the forums then reopen and/or resubmit once things are firmed up?

phaller · 2013-02-06T22:07:30Z

Yes, I think hashing out the design (both on the forums and using new PRs) would be good, especially since TaskSupport is going to play a much bigger role in the public API, so we should review its API as well I think.

heathermiller reviewed Jan 21, 2013
View reviewed changes

phaller reviewed Jan 25, 2013
View reviewed changes

SI-6998 Improvements to the task-support API

6b88596

paulp reviewed Jan 25, 2013
View reviewed changes

phaller reviewed Feb 1, 2013
View reviewed changes

JamesIry closed this Feb 6, 2013

scabug mentioned this pull request Apr 7, 2017

Fix parallel collection's configuration API (task support) scala/bug#6998

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SI-6998 Improvements to the task-support API #1937

SI-6998 Improvements to the task-support API #1937

soc commented Jan 20, 2013

soc commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

heathermiller commented Jan 21, 2013

heathermiller Jan 21, 2013

heathermiller Jan 21, 2013

adriaanm Jan 21, 2013

jsuereth Jan 25, 2013

soc commented Jan 21, 2013

phaller Jan 25, 2013

soc commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

paulp Jan 25, 2013

scala-jenkins commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

axel22 commented Jan 28, 2013

JamesIry commented Feb 1, 2013

phaller Feb 1, 2013

phaller commented Feb 1, 2013

soc commented Feb 1, 2013

soc commented Feb 1, 2013

JamesIry commented Feb 6, 2013

phaller commented Feb 6, 2013

SI-6998 Improvements to the task-support API #1937

SI-6998 Improvements to the task-support API #1937

Conversation

soc commented Jan 20, 2013

soc commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

scala-jenkins commented Jan 20, 2013

heathermiller commented Jan 21, 2013

heathermiller Jan 21, 2013

Choose a reason for hiding this comment

heathermiller Jan 21, 2013

Choose a reason for hiding this comment

adriaanm Jan 21, 2013

Choose a reason for hiding this comment

jsuereth Jan 25, 2013

Choose a reason for hiding this comment

soc commented Jan 21, 2013

phaller Jan 25, 2013

Choose a reason for hiding this comment

soc commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

paulp Jan 25, 2013

Choose a reason for hiding this comment

scala-jenkins commented Jan 25, 2013

scala-jenkins commented Jan 25, 2013

axel22 commented Jan 28, 2013

JamesIry commented Feb 1, 2013

phaller Feb 1, 2013

Choose a reason for hiding this comment

phaller commented Feb 1, 2013

soc commented Feb 1, 2013

soc commented Feb 1, 2013

JamesIry commented Feb 6, 2013

phaller commented Feb 6, 2013