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.

Sign up for GitHub

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

Jump to bottom

Replace cfor with fastFor #1077

Merged
merged 70 commits into from
Oct 19, 2021
Merged

Replace cfor with fastFor #1077

merged 70 commits into from
Oct 19, 2021

Conversation

armanbilge
Copy link
Member

Based against #1067. Opening temporarily for CI before these get merged there.

cquiroz and others added 30 commits September 12, 2021 16:43
@cquiroz
Port macros/util/platform to build in Scala 3
720bee7 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Port core to dotty
34e7a0c 
Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@cquiroz
Work on the examples
93a8887 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
wi
2595a4f 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Port laws
0d187c7 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Base test port
473bf1d 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Progress
61ea068 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
cfor macro
7b15c82 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Port more tests
5b905fb 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More tests ported
d5afb83 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Syntax test
7e1c1db 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More progress
427dfb9 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Safe rational
e96f5ad 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Better Checked macro
dc52415 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Checked work
cbd7569 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
mor progregs
95c049d 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
w
06b9cf2 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fix checked
e4ccf77 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
wip3
01fd174 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More tests
054508d 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More tests
4b6738f 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Literal macros
e10df31 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
wip
e352d6e 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Parity
76cc431 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fix build
41a43f8 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Complete literal macros
80dc82e 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fixed more tests
77b2245 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More tests
d1b6eff 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More refinements
a0a18fe 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More fixes
8130a21 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
cquiroz and others added 19 commits September 24, 2021 15:57
@cquiroz
Remove tryOrReturn on scala-3
61e6bb9 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Revert to implicit conversions for syntax
a22f582 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Progress
6283538 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
More progress
dc91ecb 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Code cleanup
bc17356 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fix for Checked failing in a border case in scala 3
ac301a4 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fix code format
0b027f7 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Include reference to the dotty discussion about Checked
079dd27 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Rename cfor to fastFor and deprecate on scala-2
7bf3b7d 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Avoid double conversions on short and byte
b4c1945 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Remove package object unicode from the scala-3 side and add a sanity …
bcf1b4c 
…check test

Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Reformat code
6f673cd 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz
Fixed typo
80f3dbb 
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@armanbilge
Revert "Rename cfor to fastFor and deprecate on scala-2"
fe5db64 
This reverts commit 7bf3b7d.
@armanbilge
Make CforSuite to Scala 2 only
3013e08
@armanbilge
Switch from cfor to fastFor
6308677
@armanbilge
Fix examples project compile
6048a58
@armanbilge
Fix benchmark compile
0ecbd00
@armanbilge
Deprecate cfor, add scaladocs to fastFor
b968633
@armanbilge armanbilge marked this pull request as ready for review October 17, 2021 01:32
armanbilge
armanbilge commented Oct 17, 2021
View reviewed changes
core/src/main/scala-2/spire/syntax/Syntax.scala Outdated
Comment on lines 280 to 290
/**
* The `fastFor` macro will replace the `cfor` macro in Scala 3.
* Note that `fastFor` has simpler semantics than `cfor` and in general is _not_ equivalent
* to inlining a while-loop, particularly with respect to closures.
* This change is unlikely to affect typical use-cases, however.
*
* The implementation of `fastFor` provided for Scala 2 is _not_ a macro but is a "reference" implementation
* with semantics matching the Scala 3 macro.
* If you are on Scala 2 and concerned about performance you should continue using `cfor`.
*/
def fastFor[A](init: A)(test: A => Boolean, next: A => A)(body: A => Unit): Unit =
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts?

@armanbilge
Copy link
Member Author

Oh, I just realized, I didn't think this through. I just replaced all internal uses of cfor with fastFor which means there will be a performance hit for Scala 2.

@armanbilge
Copy link
Member Author

I did a quick benchmark ...

[info] Benchmark                                  (size)  Mode  Cnt        Score         Error  Units
[info] CForBenchmarks.doCForGcd                  1000000  avgt    3   893577.916 ± 1440594.362  us/op
[info] CForBenchmarks.doCForIntArrayMultiply     1000000  avgt    3     5372.725 ±   17842.561  us/op
[info] CForBenchmarks.doCForMin                  1000000  avgt    3     4580.879 ±   33360.156  us/op
[info] CForBenchmarks.doCForOr                   1000000  avgt    3     5618.975 ±   47778.820  us/op
[info] CForBenchmarks.doFastForGcd               1000000  avgt    3  3551141.142 ± 9913377.889  us/op
[info] CForBenchmarks.doFastForIntArrayMultiply  1000000  avgt    3    23034.833 ±   39622.191  us/op
[info] CForBenchmarks.doFastForMin               1000000  avgt    3   322733.005 ±  109521.168  us/op
[info] CForBenchmarks.doFastForOr                1000000  avgt    3   253956.971 ± 1861700.948  us/op

@armanbilge
Copy link
Member Author

Ok, I created spire-private shims for cfor in Scala 3 and reverted all internal uses of fastFor back to cfor. This way we preserve performance on Scala 2.

@cquiroz cquiroz merged commit efcd74f into typelevel:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@armanbilge @cquiroz