Make implicit function variables safe as arguments (fixes #10889) #13750

elfprince13 · 2021-10-14T20:29:29Z

Address #10889 by adding an optimizer pass to remove synthesized contextual closures if they wrap a context function of the expected type. Add a runtime test to probe for stackoverflow if this optimization is not performed.

Fixes #10889

… contextual closures if they wrap a context function of the expected type. Add a runtime test to probe for stackoverflow if this optimization is not performed.

elfprince13 · 2021-10-14T20:51:18Z

CLA is now signed.

odersky

Mostly stylistic comments, and some explanatory comments should be added. Otherwise code LGTM.

odersky · 2021-10-15T17:48:59Z

compiler/src/dotty/tools/dotc/transform/ElimRedundantContextualClosure.scala

+import reporting.trace
+import config.Printers.{ transforms => debug }
+
+/** Abstract base class of ByNameClosures and ElimByName, factoring out the


Copy paste error here. The phase needs an detailed explanation.

odersky · 2021-10-15T17:49:51Z

compiler/src/dotty/tools/dotc/transform/ElimRedundantContextualClosure.scala

+
+      def transformArg(arg: Tree, formal: Type): Tree = {
+        val veryFormal = formal.widenDealias
+        if(defn.isContextFunctionType(veryFormal) && untpd.isContextualClosure(arg)) {


Use Scala 3 conditionals in new code. Never use if( without a space.

odersky · 2021-10-15T17:52:58Z

compiler/src/dotty/tools/dotc/transform/ElimRedundantContextualClosure.scala

+    trace(s"transforming ${tree.show} at phase ${ctx.phase}", show = true) {
+
+      def transformArg(arg: Tree, formal: Type): Tree = {
+        val veryFormal = formal.widenDealias


Why veryFormal? Maybe formalw, or simply formal1?

odersky · 2021-10-15T17:58:12Z

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -73,6 +73,7 @@ class Compiler {
         new ProtectedAccessors,     // Add accessors for protected members
         new ExtensionMethods,       // Expand methods of value classes with extension methods
         new UncacheGivenAliases,    // Avoid caching RHS of simple parameterless given aliases
+         new ElimRedundantContextualClosure, // Unwrapped context closures if the contents is already a context closure of compatible type


I'd shorten the name to ElimContextClosures. Also, the comment should be active form.

Suggested change

new ElimRedundantContextualClosure, // Unwrapped context closures if the contents is already a context closure of compatible type

new ElimContextClosures, // Unwrap context closure arguments that contain already a context closure of compatible type

Suggested change

new ElimRedundantContextualClosure, // Unwrapped context closures if the contents is already a context closure of compatible type

new ElimRedundantContextualClosure, // Unwrapped context closures if the contents is already a context closure of compatible type

odersky · 2021-10-15T17:58:57Z

compiler/src/dotty/tools/dotc/transform/ElimRedundantContextualClosure.scala

+          val bodyIsContextual = defn.isContextFunctionType(underlyingBodyType)
+          val bodyTypeMatches = TypeComparer.isSubType(underlyingBodyType, veryFormal)
+          if(bodyIsContextual && bodyTypeMatches) {
+            body


Drop the redundant braces. We tend to use Scala 3 style throughout in new code.

odersky · 2021-10-15T18:01:09Z

test performance please

elfprince13 · 2021-10-15T18:12:39Z

Is there a standard methodology I should follow for the performance testing?

odersky · 2021-10-15T18:18:31Z

That was a command to the bot 😀 Performance test should start automatically. But something seems to be wrong again /cc @anatoliykmetyuk

…dback on scala#13750

elfprince13 · 2021-10-15T18:55:16Z

I believe all comments have now been addressed. =)

anatoliykmetyuk · 2021-10-15T19:55:33Z

test performance please

dottybot · 2021-10-15T19:57:14Z

performance test scheduled: 1 job(s) in queue, 0 running.

anatoliykmetyuk · 2021-10-15T20:00:17Z

Looks like the bot crashed again. Last fix wasn't addressing the current cause of crash. I've restarted the bot and will investigate more after the weekend.

dottybot · 2021-10-15T21:12:03Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/13750/ to see the changes.

Benchmarks is based on merging with master (54ff628)

compiler/src/dotty/tools/dotc/Compiler.scala

compiler/src/dotty/tools/dotc/transform/ElimContextClosures.scala

odersky · 2021-10-16T09:47:19Z

compiler/src/dotty/tools/dotc/transform/ElimContextClosures.scala

+    trace(s"transforming ${tree.show} at phase ${ctx.phase}", show = true) {
+
+      def transformArg(arg: Tree, formal: Type): Tree = {
+        val formal1 = formal.widenDealias


In fact, widen is enough here. isContextFunction and isSubType already do dealiasing. (It's better to be consistent here since otherwise the next people looking at the compiler see multiple styles and get confused).

Presumably this applies to underlyingBodyType as well as formal1?

Sorted in 009f18b.

odersky · 2021-10-16T09:50:17Z

compiler/src/dotty/tools/dotc/transform/ElimContextClosures.scala

+      def transformArg(arg: Tree, formal: Type): Tree = {
+        val formal1 = formal.widenDealias
+        if defn.isContextFunctionType(formal1) && untpd.isContextualClosure(arg) then
+          val body = unsplice(closureBody(arg)) match {


I think it would be better to start like this

unsplice(closureBody(arg)) match case Apply(Select(body, nme.apply, _) => ... // transformation code case _ => arg

If the argument is not a nested closure there's no need to go through the following tests, right?

closureBody returns arg if arg is not a closure, so we would have to essentially reimplement the closureBody logic inline here to make sure we aren't destructuring a random Apply for no reason. I assumed checking the outer structure first with this if would be more efficient / cleaner.

Sorry, I meant: after the if defn.isContextFunctionType ..., instead of val body = ... continue with what I suggested.

oh, I see now, yes. Should be addressed in 009f18b.

Comment formatting + typos. Co-authored-by: odersky <[email protected]>

scala#13750

elfprince13 · 2021-11-29T17:57:38Z

@odersky - I believe all requested changes have been addressed.

som-snytt · 2021-11-29T19:23:07Z

tests/run/contextual-closure-unwrapping.scala

+      // (1 byte / frame is conservative even for a z80)
+      retryN(maxStackSize)(testTxn.asInstanceOf[AtomicOp[Boolean]]) == safeRetryN(maxStackSize)(testTxn)
+    } catch {
+      case e:StackOverflowError =>


Isn't it easier to just probe stack depth with new Throwable().getStackTrace? for n and n+1 recursions.

(Generally wary of tests for SOE and OOM.)

I'm not sure I see what you have in mind, can you be a little bit more explicit?

apologies in advance if I'm missing the whole point, I'm still reading the issue. I was thinking of https://github.com/scala/scala/blob/2.13.x/test/junit/scala/collection/IteratorTest.scala#L36 although counting frames is subject to optimizations.

Ah, sure, we could have the return value from testTxn be the current stack depth and then check retryN with different iteration counts. Happy to do that if it's preferable.

odersky · 2021-12-03T11:25:31Z

Tests are failing after 009f18b, so this needs to be addressed.

elfprince13 · 2021-12-03T16:17:46Z

@odersky - do you have an alternative hypothesis as to the cause, besides one of those widenDealiases was actually needed?

odersky · 2021-12-03T19:56:40Z

No did not look into it

On Fri, 3 Dec 2021 at 17:17, Thomas Dickerson ***@***.***> wrote: @odersky <https://github.com/odersky> - do you have an alternative hypothesis as to the cause, besides one of those widenDealiases was actually needed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13750 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVQHAQ5IEPLMOUU627TUPDUTJANCNFSM5GAP3GAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

odersky · 2022-01-03T21:43:25Z

We seem to have deeper problems here. I rebased the PR to current master but several tests fail, including run/tagless.scala. And that holds for all commits, not just the last ones.

som-snytt · 2022-01-03T23:27:43Z

Actually, I took a look and will follow up the PR. The current solution strips too much.

odersky · 2022-01-04T08:26:56Z

This could be generalized. We could add a general form of eta-reduction

     (x1, ..., xN) => f(x1, ..., xN)     --->    f

if f is a pure expression of function type and the closure is synthetic (i.e. its span is the span of f). Equally for ?=> instead of =>. This might replace the corresponding rule for cbn parameters. Such a miniphase would best be placed after erasure when all such closures have been generated.

som-snytt · 2022-01-04T13:15:47Z

In https://github.com/lampepfl/dotty/blob/master/tests/pos/i2671.scala, checking isStable helps.

The curious one is def f: (A ?=> B) = ??? where I would expect f to be unimplemented.

The change for CBN was to defer evaluation of the arg; the feature request here is "just an optimization". But I'm willing to call it eta-reduction.

The missed opportunity in Wonder Woman: 1984 was not giving Diana an assistant at the museum named "Etta Reduction".

odersky · 2022-01-04T16:17:50Z

The curious one is def f: (A ?=> B) = ??? where I would expect f to be unimplemented.

Eta reduction only applies if f is a function value, not a method. f by itself is not a value anyway.

Fixes scala#10889. Alternative to scala#13750, from which the test is taken.

Address scala#10889 by adding an optimizer pass to remove synthesized…

e30dffa

… contextual closures if they wrap a context function of the expected type. Add a runtime test to probe for stackoverflow if this optimization is not performed.

odersky requested changes Oct 15, 2021

View reviewed changes

odersky assigned elfprince13 Oct 15, 2021

elfprince13 added 3 commits October 15, 2021 18:47

Stylistic changes per feedback on scala#13750

a54bfd4

Renamed ElimRedundantContextualClosure to ElimContextClosures per fee…

54f50e5

…dback on scala#13750

Remove unused method leftover from copy-and-pasting

9133065

odersky requested changes Oct 16, 2021

View reviewed changes

elfprince13 and others added 2 commits October 16, 2021 16:11

Apply suggestions from code review (scala#13750)

e76f078

Comment formatting + typos. Co-authored-by: odersky <[email protected]>

More concise implementation, remove unneeded dealiasing per comments on

009f18b

scala#13750

dwijnand assigned odersky and unassigned elfprince13 Nov 8, 2021

som-snytt reviewed Nov 29, 2021

View reviewed changes

odersky assigned elfprince13 and unassigned odersky Dec 3, 2021

odersky added a commit to dotty-staging/dotty that referenced this pull request Jan 4, 2022

Add eta reduction miniphase

804b786

Fixes scala#10889. Alternative to scala#13750, from which the test is taken.

odersky mentioned this pull request Jan 4, 2022

Add eta reduction miniphase #14210

Merged

elfprince13 closed this Jan 11, 2022

olsdavis pushed a commit to olsdavis/dotty that referenced this pull request Apr 4, 2022

Add eta reduction miniphase

c7d268a

Fixes scala#10889. Alternative to scala#13750, from which the test is taken.

	new ElimRedundantContextualClosure, // Unwrapped context closures if the contents is already a context closure of compatible type
	new ElimContextClosures, // Unwrap context closure arguments that contain already a context closure of compatible type

Make implicit function variables safe as arguments (fixes #10889) #13750

Make implicit function variables safe as arguments (fixes #10889) #13750

Uh oh!

Conversation

elfprince13 commented Oct 14, 2021 • edited by odersky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elfprince13 commented Oct 14, 2021

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Oct 15, 2021

Uh oh!

elfprince13 commented Oct 15, 2021

Uh oh!

odersky commented Oct 15, 2021

Uh oh!

elfprince13 commented Oct 15, 2021

Uh oh!

anatoliykmetyuk commented Oct 15, 2021

Uh oh!

dottybot commented Oct 15, 2021

Uh oh!

anatoliykmetyuk commented Oct 15, 2021

Uh oh!

dottybot commented Oct 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elfprince13 commented Nov 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Dec 3, 2021

Uh oh!

elfprince13 commented Dec 3, 2021

Uh oh!

odersky commented Dec 3, 2021 via email

Uh oh!

odersky commented Jan 3, 2022

Uh oh!

som-snytt commented Jan 3, 2022

Uh oh!

odersky commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Jan 4, 2022

Uh oh!

elfprince13 commented Oct 14, 2021 •

edited by odersky

Loading

odersky commented Jan 4, 2022 •

edited

Loading