-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoid pickled tasty for some captured quote reference #12248
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
Avoid pickled tasty for some captured quote reference #12248
Conversation
test performance with #quotes please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench2.epfl.ch/12248/ to see the changes. Benchmarks is based on merging with master (bb5e140) |
test performance with #quotes please |
1 similar comment
test performance with #quotes please |
performance test scheduled: 3 job(s) in queue, 1 running. |
1 similar comment
performance test scheduled: 3 job(s) in queue, 1 running. |
performance test failed: Please check http://contact.support/pull-12248-04-28-10.57.out for more information |
@liufengyun what failed in the benchmarks? I cannot access the logs. |
From the error logs in the job processor, I can see is the following:
I checked the file
The line is at the beginning of the PR test script to get PR information. |
test performance with #quotes please |
performance test scheduled: 3 job(s) in queue, 1 running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, LGTM
if fn.symbol == defn.QuotedRuntime_exprQuote && isCaptured(ref.symbol, level + 1) => | ||
// Optimization: avoid the full conversion when capturing `x` with `x$1: Expr[X]` | ||
// in `'{x}` to `'{ ${x$1} }'` and go directly to `x$1` | ||
capturers(ref.symbol)(ref).select(nme.apply).appliedTo(quotes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the example '{ val y = $x * $x; ${ powerCode('y, n / 2) } }
, I'm wondering what's the argument to powerCode
before and after the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the test case I used to do this change.
It becomes something like
def hole0(args: Array[Any]) = x
def hole1(args: Array[Any]) =
val y$1 = args(0).asInstanceOf[Expr[Int]]
powerCode(y$1, n / 2)
unpickle('{ val y = [[ 0 | Double ]] * [[ 0 | Double ]]; [[ 1 | Double | y ]] }, List(hole0, hole1))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we had
def hole1(args: Array[Any]) =
val y$1 = args(0).asInstanceOf[Expr[Int]]
def hole3(args: Array[Any]) = y$1
powerCode(unpickle('{ [[ 0 | Double ]] }, List(hole3)), n / 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. Maybe put it in the commit message or doc for future reference.
Performance test finished successfully: Visit https://dotty-bench2.epfl.ch/12248/ to see the changes. Benchmarks is based on merging with master (c14ec43) |
test performance with #quotes please |
performance test scheduled: 2 job(s) in queue, 1 running. |
This avoids an extra pickle for
'y
in'{ val y = $x * $x; ${ powerCode('y, n / 2) } }
.