Skip to content

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

Conversation

nicolasstucki
Copy link
Contributor

This avoids an extra pickle for 'y in '{ val y = $x * $x; ${ powerCode('y, n / 2) } }.

@nicolasstucki
Copy link
Contributor Author

test performance with #quotes please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench2.epfl.ch/12248/ to see the changes.

Benchmarks is based on merging with master (bb5e140)

@nicolasstucki
Copy link
Contributor Author

test performance with #quotes please

1 similar comment
@nicolasstucki
Copy link
Contributor Author

test performance with #quotes please

@dottybot
Copy link
Member

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

1 similar comment
@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

performance test failed:

Please check http://contact.support/pull-12248-04-28-10.57.out for more information

@nicolasstucki nicolasstucki requested review from liufengyun and removed request for liufengyun April 28, 2021 09:02
@nicolasstucki nicolasstucki self-assigned this Apr 28, 2021
@nicolasstucki
Copy link
Contributor Author

@liufengyun what failed in the benchmarks? I cannot access the logs.

@liufengyun
Copy link
Contributor

From the error logs in the job processor, I can see is the following:

curl: (28) Failed to connect to api.github.com port 443: Connection timed out

I checked the file bin/pull, the following line failed:

curl "https://api.github.com/repos/lampepfl/dotty/pulls/$PR" > $output

The line is at the beginning of the PR test script to get PR information.

@liufengyun
Copy link
Contributor

test performance with #quotes please

@dottybot
Copy link
Member

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

Copy link
Contributor

@liufengyun liufengyun left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Apr 28, 2021

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))

Copy link
Contributor Author

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)

Copy link
Contributor

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.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench2.epfl.ch/12248/ to see the changes.

Benchmarks is based on merging with master (c14ec43)

@nicolasstucki nicolasstucki marked this pull request as ready for review April 28, 2021 11:23
@nicolasstucki
Copy link
Contributor Author

test performance with #quotes please

@dottybot
Copy link
Member

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

@nicolasstucki nicolasstucki merged commit c8a8e28 into scala:master Apr 28, 2021
@nicolasstucki nicolasstucki deleted the commit-avoid-creaation-of-pickles-for-captures branch April 28, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants