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

Specialization is broken for class inheriting from specialized traits with separate compilation #6035

Closed
scabug opened this issue Jul 6, 2012 · 16 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 6, 2012

Let's say we compile code like this:

// check-both.scala
trait Foo[@specialized(Int) A] {
  def foo(x: A): A
}
abstract class Inter extends Foo[Int]
class Baz extends Inter {
  def foo(x: Int) = x + 1
}
./bin/scalac -d classes/ check-both.scala

and we'll inspect methods in Baz:

// javap classes/Baz
Compiled from "check-both.scala"
public class Baz extends Inter{
    public int foo(int);
    public int foo$mcI$sp(int);
    public java.lang.Object foo(java.lang.Object);
    public Baz();
}

Now let's compile the same code in two steps:

// check.scala
trait Foo[@specialized(Int) A] {  
  def foo(x: A): A
}

abstract class Inter extends Foo[Int]

using ./bin/scalac -d classes/ check.scala
and then:

class Baz extends Inter {
  def foo(x: Int) = x + 1
}

using ./bin/scalac -cp classes/ -d classes/ check2.scala
if we then inspect byte of Baz we'll see:

Compiled from "check2.scala"
public class Baz extends Inter{
    public int foo(int);
    public java.lang.Object foo(java.lang.Object);
    public Baz();
}

Notice the difference that there's no public int foo$mcI$sp(int); generated this time. The reason for that is that we do not pickle SPECIALIZED flag that is set on type parameter symbols. Thus, if we read bytecode for Inter and Foo we'll not have SPECIALIZED flag set for parameter A and this will cause specialization to not kick in.

Now, the reason why we do not pickle SPECIALIZED flag is twofold:

  1. SPECIALIZED flag is set uncurry (see: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/transform/UnCurry.scala#L569). This is rather questionable place for that kind of code as it mutates flags and pickle will never see those changes.
  2. Even if we set the flag before symbols pickling this won't help because
    only 32 lowest bits are pickled (see https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Flags.scala#L310) and SPECIALIZED has index 40 (https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Flags.scala#L160).

The first problem has been introduced in 7bddd73 (scala/scala@7bddd73) but I don't think reverting this change makes sense for efficiency reasons. Therefore we have two choices:

  1. Change the index of SPECIALIZED flag so it gets pickled and set it before pickling happens (probably right before, it means in pickler).
  2. Check for specialized annotation while unpickling symbols and set the flag there.

I like the first solution much better. We don't want to slow down unpickling.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6035?orig=1
Reporter: @gkossakowski
Affected Versions: 2.10.0
See #5005

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@gkossakowski said:
This change is related to #5005 because once we generate correct specialized bridge methods we should unblock inlining.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@magarciaEPFL said:
In #5005 I noticed something similar, a specialized method not being generated for an anon-closure-class.

However, in that case and also here, a "correct" implementation for that method must be inherited, right? (Otherwise the bytecode verifier would complain).

Including the javap of Inter would help in visualizing the specialization vs bytecode-duplication tradeoff.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@axel22 said (edited on Jul 6, 2012 11:13:44 AM UTC):
I did the 2nd solution already here:

axel22/scala-github@514925c

Paul, since you optimized the specialized flag in pickling previously, can you comment if that is ok, or do you feel that the flag itself should be pickled?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@gkossakowski said:
Miguel, here's the relevant bytecode (with Alex's fix applied):

Compiled from "check.scala"
public abstract class Inter extends java.lang.Object implements Foo$mcI$sp
public int foo$mcI$sp(int);
  Code:
   Stack=2, Locals=2, Args_size=2
   0:	aload_0
   1:	iload_1
   2:	invokestatic	#11; //Method Foo$class.foo$mcI$sp:(LFoo;I)I
   5:	ireturn
Compiled from "check.scala"
public abstract class Foo$class extends java.lang.Object

public static int foo$mcI$sp(Foo, int);
  Code:
   Stack=2, Locals=2, Args_size=2
   0:	aload_0
   1:	iload_1
   2:	invokestatic	#12; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
   5:	invokeinterface	#18,  2; //InterfaceMethod Foo.foo:(Ljava/lang/Object;)Ljava/lang/Object;
   10:	invokestatic	#22; //Method scala/runtime/BoxesRunTime.unboxToInt:(Ljava/lang/Object;)I
   13:	ireturn
Compiled from "check2.scala"
public class Baz extends Inter
public int foo$mcI$sp(int);
  Code:
   Stack=2, Locals=2, Args_size=2
   0:	iload_1
   1:	iconst_1
   2:	iadd
   3:	ireturn

as you can see Inter just forward to Foo$class that in turns forwards to abstract, specialized method and takes care of boxing and unboxing.

The Baz class overrides specialized implementation and thus avoids boxing. I think this is really what we need.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@axel22 said:
Greg and I were checking this out and saw that there was another issue with:

trait Foo[@specialized(AnyRef, Int) A, @specialized(Boolean) B] {
  def apply(x: A): B
}


abstract class Inter extends Foo[String, Boolean]


class Baz extends Inter {
  def apply(x: String) = false
}

The apply$mcLZ$sp was not getting specialized in Baz, but just retained the bridge to apply. This should work now.

Here's the relevant commit:

axel22/scala-github@830ba25

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@paulp said:

Even if we set the flag before symbols pickling this won't help because
only 32 lowest bits are pickled

Right, that's why I wasn't too concerned about putting it in uncurry, which I agree is arbitrary.

The phase called "superaccessors" which already is less than completely descriptively named should maybe be renamed - it's the only waystation between typer and pickler, and pickler should stick to pickling.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@paulp said:
Incidentally, the flag/pickler relationship is more complicated, they are mapped in and out of the pickler space. This should make it easier than you're thinking to add it.

  private def rawPickledCorrespondence = Array(
    (IMPLICIT, IMPLICIT_PKL),
    (FINAL, FINAL_PKL),
    (PRIVATE, PRIVATE_PKL),
    (PROTECTED, PROTECTED_PKL),
    (SEALED, SEALED_PKL),
    (OVERRIDE, OVERRIDE_PKL),
    (CASE, CASE_PKL),
    (ABSTRACT, ABSTRACT_PKL),
    (DEFERRED, DEFERRED_PKL),
    (METHOD, METHOD_PKL),
    (MODULE, MODULE_PKL),
    (INTERFACE, INTERFACE_PKL)
  )
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@paulp said:
As you can see from the 12 entries above, we're only using 12 flag bits at present:

  private final val PKL_MASK       = 0x00000FFF
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@paulp said:
Well, the pickler code is unnecessarily confusing - is it only masking out those 12 bits, letting the other 20 low order bits be pickled directly, and then shuffling around those 12? Maybe it is. Well, there's no way all those flags need pickling, and there are others which do. See this comment in Unpickler:

        case EXISTENTIALtpe =>
          val restpe  = readTypeRef()
          // @PP: Where is the flag setting supposed to happen? I infer
          // from the lack of flag setting in the rest of the unpickler
          // that it isn't right here. See #4757 for the immediate
          // motivation to fix it.
          val tparams = until(end, readSymbolRef) map (_ setFlag EXISTENTIAL)
          newExistentialType(tparams, restpe)
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 6, 2012

@paulp said:
It looks like shuffling the first 12 bits is exactly what it's doing. For the love of god, WHY? Why why why...

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 12, 2012

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 12, 2012

@gkossakowski said:
This is not fixed in latest master. Given Martin's fix to Flags all we need is to set the SPECIALIZED flag before pickler. Martin suggested superaccessor (as the place that already had a bunch of unrelated stuff).

I'll try to do that.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 12, 2012

@paulp said:
Right, superaccessors is the only stage between typer and pickler - oh I guess there's patmat now. Anyway, it should get a name reflecting that (maybe posttyper, analogous with posterasure.)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 12, 2012

@gkossakowski said:
The pull request landed here: scala/scala#890

@scabug scabug closed this Jul 17, 2012
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 18, 2012

@adriaanm said:
it ended up being merged as scala/scala#915

@scabug scabug added this to the 2.10.0-M5 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.