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

Inconsistent behavior of the miniboxing plugin code transformations #242

Open
lastland opened this issue Sep 11, 2015 · 8 comments
Open

Comments

@lastland
Copy link
Contributor

The following code will report that Fractional has been upgraded to MiniboxedFractional, but it's not.

object Main {
  def add[@miniboxed T](a: T, b: T)(implicit num: Fractional[T]) = num.plus(a, b)
  def main(args: Array[String]) {
    println(add(BigDecimal(2.0), BigDecimal(1.0)))
  }
}

A proof is that there's a bug in MiniboxedFractional as shown in #241 , but the bug does not appear in the above program.

If you look into InteropTreeTransformer, you will find that the transformer will trigger a warning about the upgrades, but actually does nothing in transforming tree0:

case _ if (TypeClasses.contains(tree0.symbol)) =>
  val targs  = tree0.tpe.dealiasWiden.typeArgs
  assert(targs.length == 1, "targs don't match for " + tree0 + ": " + targs)
  val targ = targs(0)
  // warn only if the type parameter is either a primitive type or a miniboxed type parameter
  if (ScalaValueClasses.contains(targ.typeSymbol) || targ.typeSymbol.deSkolemize.hasAnnotation(MiniboxedClass))
    // TODO:
    // (1) move to commit
    // (2) use a warning, no custom code!
    minibox.suboptimalCodeWarning(tree0.pos, "Upgrade from " + tree0.symbol.fullName + "[" + targ + "]" + " to " + TypeClasses(tree0.symbol).fullName + "[" + targ + "] to benefit from miniboxing specialization. " )
  super.transform(tree0)
@VladUreche
Copy link
Member

@lastland yes, it's not yet automated. To be honest, seeing all the bugs MiniboxedFunctions triggered, I think it's best if we leave it that way. Do you think it's too difficult to make the replacements by hand?

Or, maybe, would putting the classes in the scala package directly make it easier?

@lastland
Copy link
Contributor Author

I don't think that it would be more difficult than adding @miniboxeds by hand, but this could not be automated by using the minibox:mark-all compiler option, right?

@VladUreche
Copy link
Member

No, it can't be automated. Now I'm see what you mean, the different transformations are inconsistent:

  • @miniboxed annotations need to be introduced by hand, but can be automated via -P:minibox:mark-all
  • Numeric and the other type classes have to be introduced by hand, but can't be automated via -P:minibox:mark-all
  • Tuple accessors and Functions are automatically converted, and can only be turned off using -P:minibox:off.

I agree this causes confusion. What do you think would be a good approach?

@VladUreche
Copy link
Member

I imagine it could all start from the @miniboxed annotation:

  • Numeric and other type classes are replaced automatically by MiniboxedNumeric and co.
  • Functions are replaced when their type arguments are marked as @miniboxed
  • Tuple accessors are replaced when the two components are marked as @miniboxed

Then, if you don't add any @miniboxed annotation, nothing is changed. If you do, either manually or via -P:minibox:mark-all everything is consistently changed.

What do you think?

@lastland
Copy link
Contributor Author

I agree. It feels most natural from a user's perspective (well, from my perspective 😜).

@VladUreche
Copy link
Member

Thanks @lastland!
How would you feel about moving this to the next milestone? I don't think that I'm able to pull it off quickly and I really want to get to the 0.4 release out the door.

@VladUreche VladUreche changed the title Classes under runtime.math are not used when marked miniboxed Inconsistent behavior of the miniboxing plugin code transformations Sep 13, 2015
@lastland
Copy link
Contributor Author

Of course. No worries. 😄

@VladUreche VladUreche modified the milestones: 0.5, 0.4 Sep 13, 2015
@VladUreche
Copy link
Member

Cool, thanks @lastland! I will update the behavior for the next milestone. In the meantime, if you notice any strange behavior/need help getting the miniboxing plugin to do what you want, ask on gitter anytime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants