Skip to content

Don't box some simple primitives (forward-port)#9261

Merged
dwijnand merged 4 commits intoscala:2.13.xfrom
dwijnand:2.13/dont-box-Double-NaN
Feb 1, 2021
Merged

Don't box some simple primitives (forward-port)#9261
dwijnand merged 4 commits intoscala:2.13.xfrom
dwijnand:2.13/dont-box-Double-NaN

Conversation

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Oct 22, 2020

don't box primitive values to call static methods on java.lang.Double/Float/Integer etc

don't box primitives to call hashCode/toString
copy with trees with side effect
add tests to verify no boxing

(cherry picked from commit 95f0ba0)
Forward-port of #9102
Refs scala/scala-dev#721

@scala-jenkins scala-jenkins added this to the 2.13.4 milestone Oct 22, 2020
@dwijnand dwijnand mentioned this pull request Oct 22, 2020
Closed
@lrytz

This comment has been minimized.

Sign in to view
@dwijnand dwijnand modified the milestones: 2.13.4, 2.13.5 Oct 23, 2020
@dwijnand dwijnand marked this pull request as draft October 23, 2020 09:53
@dwijnand dwijnand force-pushed the 2.13/dont-box-Double-NaN branch from 08da561 to 5e5c127 Compare January 27, 2021 16:56
@dwijnand
Copy link
Member Author

dwijnand commented Jan 27, 2021 •
edited
Loading

  1. Looks like I messed up the cherry-pick, moving from junit/testing to testkit/testkit I hadn't noticed the coster.short/etc value changes.
  2. The removal of the signum changes and non-addition of the new sign method in the Rich* classes means these methods box, because they're inherited methods... which really sucks. It sucks even more so because now we can't add them... because of forwards-compatibility (until/if we get @inheritsSignature - Add @inheritSignature for forwards binary compatible overrides #9141 nope, not even then)...

So I'll be adjusting the test expectations in addition to cherry-picking.

@dwijnand
Copy link
Member Author

dwijnand commented Jan 27, 2021

Ironically overriding signum in RichDouble/Float for the purposes of deprecating them (in addition to my changing them back to math.signum) makes the methods fast but wrong, because they don't box...

@dwijnand dwijnand marked this pull request as ready for review January 27, 2021 18:42
@dwijnand dwijnand force-pushed the 2.13/dont-box-Double-NaN branch 2 times, most recently from e3863f5 to 7549068 Compare January 27, 2021 21:18
mkeskells and others added 3 commits January 28, 2021 10:24
@mkeskells @dwijnand
don't box primitive values to call static methods on java.lang.Double…
16c7861 
…/Float/Integer etc

don't box primitives to call hashCode/toString
copy with trees with side effect
add tests to verify no boxing

(cherry picked from commit 95f0ba0)
@dwijnand
Fix no-boxing tests by changing the library...
7b87a01
@dwijnand
... revert back to fix MiMa, regressing the tests
0e74a14
@dwijnand dwijnand force-pushed the 2.13/dont-box-Double-NaN branch from 7549068 to 0e74a14 Compare January 28, 2021 10:27
lrytz
lrytz reviewed Jan 29, 2021
View reviewed changes
@dwijnand dwijnand merged commit 0ab9086 into scala:2.13.x Feb 1, 2021
@dwijnand dwijnand deleted the 2.13/dont-box-Double-NaN branch February 1, 2021 09:44
@dwijnand dwijnand mentioned this pull request Feb 4, 2021
Merged
@SethTisue SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Feb 9, 2021
hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this pull request May 2, 2025
@dwijnand
Merge pull request scala/scala#9261 from dwijnand/2.13/dont-box-Doubl…
e9ee2d9 
…e-NaN
hamzaremmal pushed a commit to scala/scala3 that referenced this pull request May 7, 2025
@dwijnand
Merge pull request scala/scala#9261 from dwijnand/2.13/dont-box-Doubl…
ed8daf4 
…e-NaN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz left review comments

Assignees

@dwijnand dwijnand

Labels

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

Projects

None yet

Milestone

2.13.5

Development

Successfully merging this pull request may close these issues.

5 participants

@dwijnand @lrytz @SethTisue @scala-jenkins @mkeskells