Skip to content

issue 12062 - don't box primitives for some simple cases [ci: last-only]#9102

Merged
lrytz merged 1 commit intoscala:2.12.xfrom
mkeskells:2.12.x_DoubleOps
Sep 3, 2020
Merged

issue 12062 - don't box primitives for some simple cases [ci: last-only]#9102
lrytz merged 1 commit intoscala:2.12.xfrom
mkeskells:2.12.x_DoubleOps

Conversation

@mkeskells
Copy link
Contributor

@mkeskells mkeskells commented Jul 3, 2020 •
edited by lrytz
Loading

undo the wrapping of Predef.double2Double(d).isNaN() to java.lang.Double.isNaN
undo the wrapping of Predef.float2float(f).isNaN() to java.lang.Float.isNaN

do the same for other similar methods

  • cope with all of the other similar Predef boxing conversions
  • cope with similar boxing for calls to toStringand hashCode
  • add allocation tests to check for boxing

allocation tests are not great for boxing checks for example in Boolean, but it seemed the simplest way to check for the common cases

fixes scala/bug#12062

@scala-jenkins scala-jenkins added this to the 2.12.13 milestone Jul 3, 2020
@mkeskells mkeskells requested review from hrhino and lrytz July 3, 2020 16:27
@mkeskells
Copy link
Contributor Author

mkeskells commented Jul 7, 2020

/rebuild

@mkeskells mkeskells changed the title issue 12062 - don't box doubles to call static methods on java.lang.Double issue 12062 - don't box doubles/floats to call static methods on java.lang.Double/Float Aug 11, 2020
hrhino
hrhino reviewed Aug 11, 2020
View reviewed changes
@mkeskells mkeskells changed the title issue 12062 - don't box doubles/floats to call static methods on java.lang.Double/Float issue 12062 - don't box primitives for some simple cases Aug 12, 2020
@mkeskells mkeskells requested a review from hrhino August 12, 2020 07:21
dwijnand
dwijnand reviewed Aug 12, 2020
View reviewed changes
dwijnand
dwijnand reviewed Aug 12, 2020
View reviewed changes
dwijnand
dwijnand reviewed Aug 12, 2020
View reviewed changes
@sjrd
Copy link
Member

sjrd commented Aug 12, 2020

IMO it's worrying that we are now solving an issue with more code in the compiler where a 100% library-based solution would have done the job, and more reliably so:
2.13.x...sjrd:efficient-rich-primitives

Of course, there's the binary compatibility problem now, but still...

@dwijnand
Copy link
Member

dwijnand commented Aug 12, 2020

IMO it's worrying that we are now solving an issue with more code in the compiler where a 100% library-based solution would have done the job, and more reliably so:
2.13.x...sjrd:efficient-rich-primitives

Of course, there's the binary compatibility problem now, but still...

Wouldn't work, see scala/bug#12062:

Using Predef.doubleWrapper(1.0).isNaN would be preferable because RichDouble is a value class. However, doubleWrapper is a lower priority implicit than double2Double because it's defined in Predef's parent LowPriorityImplicits.

dwijnand
dwijnand reviewed Aug 12, 2020
View reviewed changes
@mkeskells mkeskells force-pushed the 2.12.x_DoubleOps branch 2 times, most recently from 6816027 to 1dc4ff5 Compare August 12, 2020 23:09
@mkeskells mkeskells mentioned this pull request Aug 13, 2020
Merged
lrytz
lrytz reviewed Aug 13, 2020
View reviewed changes
src/compiler/scala/tools/nsc/transform/CleanUp.scala Outdated
//new Predef.float2Float(x).intValue() -> x.toInt()
//new Predef.float2Float(x).longValue() -> x.toLong()
//new Predef.float2Float(x).floatValue() -> x.toFloat()
//new Predef.float2Float(x).doubleValue() -> x.toDouble()
Copy link
Member

@lrytz lrytz Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle conversions from the other primitive types? 1.longValue has the same issue, it compiles to int2Integer(1).longValue (boxes), not intWrapper(1).longValue (doesn't box).

On the other hand, is it worth handling these conversions? Are people actually using xValue instead of toX?

Copy link
Contributor Author

@mkeskells mkeskells Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems easy enough to do. will squash and add this on top to review

@mkeskells
Copy link
Contributor Author

mkeskells commented Aug 13, 2020

@lrytz squashed and updated with your comments
added conversion for the java numbers, seemed easy enough, and would probably get used then people are converting from java code

lrytz
lrytz requested changes Aug 14, 2020
View reviewed changes
@mkeskells mkeskells requested a review from lrytz August 14, 2020 12:04
@mkeskells mkeskells force-pushed the 2.12.x_DoubleOps branch 2 times, most recently from 66af97e to af5e717 Compare August 16, 2020 22:05
retronym
retronym reviewed Aug 17, 2020
View reviewed changes
retronym
retronym reviewed Aug 17, 2020
View reviewed changes
lrytz
lrytz reviewed Aug 17, 2020
View reviewed changes
@mkeskells mkeskells requested a review from lrytz August 17, 2020 16:39
@mkeskells mkeskells changed the title issue 12062 - don't box primitives for some simple cases issue 12062 - don't box primitives for some simple cases [ci: last-only] Aug 17, 2020
@lrytz lrytz force-pushed the 2.12.x_DoubleOps branch from 2a13c63 to 5ad8df9 Compare August 20, 2020 13:20
@lrytz
Copy link
Member

lrytz commented Aug 20, 2020

(rebased on current 2.12.x)

lrytz
lrytz reviewed Aug 20, 2020
View reviewed changes
lrytz
lrytz approved these changes Aug 20, 2020
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pushed a tiny cleanup. We should squash everything before merging.

@SethTisue SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Aug 20, 2020
@mkeskells
don't box primitive values to call static methods on java.lang.Double…
95f0ba0 
…/Float/Integer etc

don't box primitives to call hashCode/toString
copy with trees with side effect
add tests to verify no boxing
@lrytz lrytz merged commit 975a9b6 into scala:2.12.x Sep 3, 2020
@dwijnand dwijnand mentioned this pull request Sep 17, 2020
Closed
@dwijnand dwijnand mentioned this pull request Oct 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@retronym retronym retronym left review comments

@dwijnand dwijnand dwijnand left review comments

@lrytz lrytz lrytz approved these changes

@hrhino hrhino Awaiting requested review from hrhino

Assignees

No one assigned

Labels

performance the need for speed. usually compiler performance, sometimes runtime performance.

Projects

None yet

Milestone

2.12.13

Development

Successfully merging this pull request may close these issues.

8 participants

@mkeskells @sjrd @dwijnand @lrytz @retronym @hrhino @SethTisue @scala-jenkins