SPARK-2519 part 2. Remove pattern matching on Tuple2 in critical section...

[sryza]

...s of CoGroupedRDD and PairRDDFunctions

This also removes an unnecessary tuple creation in cogroup.

[rxin]

can we save the key into a variable here so we don't call _1 twice?

I was looking at the bytecode generated by pattern matching versus non-pattern-matching, and the main difference is that we bypass some unnecessary branches:

    final def apply(x0$1: Tuple2): Unit = {
      case <synthetic> val x1: Tuple2 = x0$1;
      case4(){
        if (x1.ne(null))
          {
            val k: Int = x1._1$mcI$sp();
            val v: Int = x1._2$mcI$sp();
            matchEnd3({
              scala.this.Predef.println(scala.Int.box(k.+(v)));
              scala.runtime.BoxedUnit.UNIT
            })
          }
        else
          case5()
      };
      case5(){
        matchEnd3(throw new MatchError(x1))
      };
      matchEnd3(x: runtime.BoxedUnit){
        ()
      }

[aarondav]

If we're using functional style anyway, do we really expect to see any sort of remotely noticeable gain from doing these kinds of optimizations?

[rxin]

It's really hard to tell. I think it is really hard to tell when doing microbenchmarks because branch prediction takes care of the extra branches. But I don't know how this impacts real runtime in a real workload without a lot more instrumentation. My opinion is since it is a small change, it's ok to just do it this way. If it is a big refactoring, that's a very different story and deserves more profiling.

[mateiz]

Removing the call to _1 isn't hugely helpful IMO; accessor methods get inlined very fast. However, pattern matching adds a whole bunch of potential throw sites and branches and things like that, and we've noticed problems in the past.

[aarondav]

This I'm sure has been done a million times, but one more for good luck -- here is a foreach { xy => xy._1 + xy._2 } versus map { case (x, y) => x + y }:
http://www.diffchecker.com/vtv6cptx

No new virtual function calls, but one new branch (trivially branch predictable) and one new throw (which will never be invoked), and several store/loads. (Perhaps I'm misreading the bytecode, but isn't astore_2 followed by aload_2 a no-op? Edit: I guess it does change the state of variable 2, so is not a no-op.)

[mateiz]

I think we tested this in the past and saw a difference, perhaps because it can now throw. But I don't remember the details. Weird that it's also doing those stores.

[rxin]

There are all kinds of stuff that's really hard to profile at runtime. For example, the JVM might decide not to JIT the code because it is longer. It might decide not to do it because of exceptions. Branch prediction might stop working because it has too many branches. Etc.

Again, I'm only in favor of this change because it is small, and it might have impact. If it is a big ugly change, I would be against it without profiling.

[sryza]

Also on removing the calls to _1, these values should be in cache, so accesses will be really fast. Will hold off on these for now, but happy to make the change if y'all want.

[SparkQA]

QA tests have started for PR 1447. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16771/consoleFull

[SparkQA]

QA tests have started for PR 1447. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16773/consoleFull

[SparkQA]

QA results for PR 1447:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16771/consoleFull

[SparkQA]

QA results for PR 1447:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16773/consoleFull

[mateiz]

This looks okay to me as is, @rxin what do you think?

[sryza]

Upmerged

[SparkQA]

QA tests have started for PR 1447. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16821/consoleFull

[SparkQA]

QA results for PR 1447:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16821/consoleFull

[rxin]

Merging this in master.