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

No Sync for compileLoop #1553

Conversation

@diesalbla
Copy link
Contributor

commented Aug 11, 2019

The Sync is only needed in the call to suspend, to recur through the GetScope case.

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

@pchlupacek The git history indicates that the F.suspend was introduced in this commit. Can you remember why was it necessary, and if it still is? So far, the tests pass.

@pchlupacek

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Yes I bet this was stack safety issue. Perhaps due to changes to compiler this is not necessary, but I would suggest to do more tests with deep translate and interpreter nesting

@djspiewak

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Just looking at it, without looking at the bytecode, it doesn't look necessary for stack safety, or anything else really. We should sanity check the compiled invocation. I've seen the compiler sometimes fail to recognize a tail call on a function which encodes a gadt skolem, but that bug may also be fixed.

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@djspiewak I think that those problems in the compiler, between tail calls and GADT skolems, is what I found in #1529 (comment).

@djspiewak

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

It works on 2.12:

     439: aload         18
     441: instanceof    #50                 // class fs2/internal/Algebra$GetScope
     444: ifeq          473
     447: aload_1
     448: aload         17
     450: invokevirtual #466                // Method fs2/internal/FreeC$ViewL$View.next:()Lscala/Function1;
     453: getstatic     #472                // Field fs2/internal/FreeC$Result$.MODULE$:Lfs2/internal/FreeC$Result$;
     456: aload_1
     457: invokevirtual #767                // Method fs2/internal/FreeC$Result$.pure:(Ljava/lang/Object;)Lfs2/internal/FreeC$Result;
     460: invokeinterface #479,  2          // InterfaceMethod scala/Function1.apply:(Ljava/lang/Object;)Ljava/lang/Object;
     465: checkcast     #79                 // class fs2/internal/FreeC
     468: astore_2
     469: astore_1
     470: goto          0
@djspiewak

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

And on 2.11:

     440: aload         16
     442: instanceof    #186                // class fs2/internal/Algebra$GetScope
     445: ifeq          474
     448: aload_1
     449: aload         15
     451: invokevirtual #362                // Method fs2/internal/FreeC$ViewL$View.next:()Lscala/Function1;
     454: getstatic     #370                // Field fs2/internal/FreeC$Result$.MODULE$:Lfs2/internal/FreeC$Result$;
     457: aload_1
     458: invokevirtual #548                // Method fs2/internal/FreeC$Result$.pure:(Ljava/lang/Object;)Lfs2/internal/FreeC$Result;
     461: invokeinterface #379,  2          // InterfaceMethod scala/Function1.apply:(Ljava/lang/Object;)Ljava/lang/Object;
     466: checkcast     #127                // class fs2/internal/FreeC
     469: astore_2
     470: astore_1
     471: goto          0

I really don't think the suspend is necessary. I'll check 2.13 to be thorough, but I'm sure the behavior hasn't regressed.

@djspiewak

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

And 2.13:

     436: goto          439
     439: aload         18
     441: instanceof    #50                 // class fs2/internal/Algebra$GetScope
     444: ifeq          473
     447: aload_1
     448: aload         17
     450: invokevirtual #456                // Method fs2/internal/FreeC$ViewL$View.next:()Lscala/Function1;
     453: getstatic     #462                // Field fs2/internal/FreeC$Result$.MODULE$:Lfs2/internal/FreeC$Result$;
     456: aload_1
     457: invokevirtual #755                // Method fs2/internal/FreeC$Result$.pure:(Ljava/lang/Object;)Lfs2/internal/FreeC$Result;
     460: invokeinterface #469,  2          // InterfaceMethod scala/Function1.apply:(Ljava/lang/Object;)Ljava/lang/Object;
     465: checkcast     #79                 // class fs2/internal/FreeC
     468: astore_2
     469: astore_1
     470: goto          0

@diesalbla diesalbla marked this pull request as ready for review Aug 12, 2019

@SystemFw
Copy link
Member

left a comment

I'd probably go ahead with this, even though it doesn't solve the problem of compile requiring Sync.
I'm assuming that the suspend was there for stack safety at the time, and the analysis by Daniel seems enough proof that it's unneeded

@mpilquist mpilquist merged commit 3f95c3c into functional-streams-for-scala:series/1.1 Aug 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@diesalbla diesalbla deleted the diesalbla:no_sync_in_compile branch Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.