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

Experimental Streams using quotes #4317

Merged
merged 28 commits into from Jun 13, 2018

Conversation

Projects
None yet
4 participants
@biboudis
Copy link
Contributor

commented Apr 13, 2018

Port of the strymonas library as described in O. Kiselyov et al., Stream fusion, to completeness (POPL 2017) using Dotty's staging facility.

Ideas:

  • design a cleaner API, possibly investigate if it could be part of collections
  • experiment with alternative code generation schemes, e.g., generate fused code run wtih OpenCL/CUDA bindings
  • multidimensional types
producer.init(st => '{
Var('{ (_: Unit) => ()}){ advf => {
Var('{ true }) { hasNext => {
// Var('{ null.asInstanceOf[A] }) { curr => {

This comment has been minimized.

Copy link
@biboudis

biboudis Apr 19, 2018

Author Contributor

I would like to generate the following line:

var curr: A = null.asInstanceOf[A]

so I guess the only way is:

Var('{ null.asInstanceOf[A] }) { curr => ....

The minimal standalone example that corresponds to the code above is:

trait Bar[T: Type] { def meth(): Unit }
class Foo[T: Type] {
  new Bar[T] {
    def meth() = {
      Var('(null.asInstanceOf[T])) { curr => '() }
    }
  }
}

The problem is that I get the following error message and I don't know if thats a bug or I am doing something horribly wrong.

25 |    new Bar[T] {
   |        ^^^^^^
   |        constructor Bar in trait Bar does not take parameters

ping @nicolasstucki

This comment has been minimized.

Copy link
@biboudis

biboudis Apr 20, 2018

Author Contributor
  trait Bar { def meth(): Unit }
  class Foo[T: Type] {
    new Bar {
      def meth() = {
        Var('(null.asInstanceOf[T])) { curr => '() }
      }
    }
  }
Var('(null.asInstanceOf[T])) { curr => '() }
   |                                ^
   |                            access to Foo.this from wrong staging level:
   |                             - the definition is at level 0,
   |                             - but the access is at level 1.

This comment has been minimized.

Copy link
@biboudis

biboudis Apr 20, 2018

Author Contributor

Opened #4350 after investigation

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch 4 times, most recently from 55b5913 to 087a191 Apr 25, 2018

@biboudis

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

// Fixed: Note to self: the call to adv is not generated correctly in the nested case. Have to investigate

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch from efc5484 to 8fa59a6 May 3, 2018

@biboudis biboudis removed the stat:wip label May 3, 2018

@biboudis

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

@nicolasstucki parallel tests failed. I will restart the build.

@biboudis biboudis requested a review from odersky May 4, 2018

}

object Stream {
def of[A: Type](arr: Expr[Array[A]]): Stream[A] = {

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 4, 2018

Contributor

Try changing it to

def of[A: Type: Liftable](arr0: Array[A]): Stream[A] = {
  val arr = arr0.toExpr
  ...

You will need some lifter for Arrays from the library.

}

/** A stream containing the first `n` elements of this stream. */
def take(n: Expr[Int]): Stream[A] = Stream(takeRaw[Expr[A]](n, stream))

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 4, 2018

Contributor
def take(n: Int): Stream[A] = Stream(takeRaw[Expr[A]](n.toExpr, stream))

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

@nicolasstucki what’s the advantage of your proposal? At first sight it seems less powerful.

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 22, 2018

Contributor

The aim is to hide the Expr from the end user.

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

Then do we want fold to have the following signature?

def fold[W : Type](z: W, f: (W, A) => W): W

I’m not sure this is what we want?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 22, 2018

Contributor

You are right, it is not exactly what I want. We need to hide the Expr behind a macro. I will test my signature idea and then document it here if it works

@biboudis biboudis requested a review from julienrf May 9, 2018

@nicolasstucki
Copy link
Contributor

left a comment

I think we should merge like this. It will serve a great regression test. We should refine it further based on this work afterward.

@biboudis

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

I have the changes you proposed ready but I was stuck in something during the refactoring. I will post here, maybe discuss and merge.

Thanks @nicolasstucki

* {{{
* /* initialization for stream 1 */
*
* var curr = null.asInstanceOf[Int] // keeps each element from reified stream

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

This should probably be 0 instead of null. Also, it seems that curr is never updated.

This comment has been minimized.

Copy link
@biboudis

biboudis May 28, 2018

Author Contributor

👍

I was "updating" it as a comment. Fixing it to illustrate it better!

producer.init(st =>
Var('{ (_: Unit) => ()}){ nadv => {
Var('{ true }) { hasNext => {
Var('{ null.asInstanceOf[A] }) { curr => '{

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

I’m wondering if this can cause subtle bugs, since null.asInstanceOf[Int] produces 0, which is undistinguishable from the value 0 (by contrast with references, where null can always be distinguished from an object reference).

Why don’t you use Option[A] instead?

Var(z) { s: Var[W] => '{
~foldRaw[Expr[A]]((a: Expr[A]) => '{
~s.update(f(s.get, a))
}, stream)

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

Is there any difference between '{ ~s.update(…) } and s.update(…)?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 22, 2018

Contributor

It is the same

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 22, 2018

Contributor

Though I would not change it. This kind of small additions have been useful to find bugs in the implementation. I would keep it like this for as a regression test.

This comment has been minimized.

Copy link
@biboudis

biboudis May 28, 2018

Author Contributor

However, nice catch @julienrf!

@julienrf
Copy link

left a comment

This looks interesting! Do you have more insights on the generated code or on performance?

}
}

private def foldRaw[A](consumer: A => Expr[Unit], stream: StagedStream[A]): Expr[Unit] = {

This comment has been minimized.

Copy link
@julienrf

julienrf May 22, 2018

It seems that we don’t need to take stream as a parameter since it is already a parameter of the Stream class. (btw name shadowing is dangerous!)

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch from 1a637b5 to 6923432 May 28, 2018


def init(k: St => Expr[Unit]): Expr[Unit] = {
producer.init(st =>
Var('{ (_: Unit) => ()}){ nadv => {

This comment has been minimized.

Copy link
@biboudis

biboudis May 28, 2018

Author Contributor

@nicolasstucki this is the line that triggers the exception after the last rebase

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki May 28, 2018

Contributor

Fix in #4592

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch from 6fa33a0 to 2599e06 May 28, 2018

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch from 2599e06 to 6fa33a0 May 29, 2018

@biboudis biboudis force-pushed the dotty-staging:streams-dev branch from 6fa33a0 to 3c46da8 May 29, 2018

@biboudis biboudis merged commit 78a5e09 into lampepfl:master Jun 13, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@allanrenucci allanrenucci deleted the dotty-staging:streams-dev branch Jun 13, 2018

@LPTK

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2018

@biboudis very cool! Has this implementation been benchmarked? Does it expand full stream pipelines into low-level loops, as in the paper?

@biboudis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

Thanks @LPTK. Yes, by inspection, the code generates the expected imperative low-level loops. If needed we will benchmark it in the future. For now this is merely a big test case for us.

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