Stream chaining with encore primitive type #416

PhucVH888 · 2016-05-16T12:00:03Z

This implementation includes a chaining function with the signature def chain(sa:Stream A, f:A->B) : Stream B with A and B are type aliases of typedef A = int and typedef B = real.

The stream chaining function basically takes advantage of future chaining to avoid using get that is related to blocking on a future. Stream chaining attached the closure f:A->B to the stream to run when the stream (or future scons) is fulfilled.

It immediately returns the Stream B that will store the result of applying closure f:A->B. The commit works with primitive types as the baseline.

supercooldave · 2016-05-16T12:04:43Z

I reject this for the reasons we have discussed numerous times. The function chain3args() violates the stream abstraction. Stream chaining can be implemented purely using future chaining and you don't need to get in there and patch one stream onto the tail of another.

supercooldave · 2016-05-16T12:06:36Z

bundles/prototype/StreamChain/StreamChain.enc

+              end
+            nexta = embed (Stream A) scons_next(encore_ctx(),(scons_t*)#{scons}); end
+        in chain3args(nexta, nextb, f)
+  in futa ~~>fscons


The resulting future is never actually used. The code seems to be using future chaining for a side effect, rather than functionally, which seems very wrong.

You want to write a function from scons<a> to scons which

Creates a new scons object.

Checks whether the scons<a> element corresponds to the end-of-stream or not.

If it corresponds to the end of the stream, create an end-of-stream scons and return it.

Otherwise:

Applies f to the data in the scons<a> object and stores that in the scons object.

Recursively does stream chaining on the stream element of scons<a> producing the stream element of scons and stores that result in the scons object.

Returns the resulting scons object.

And chain this function onto the head of the stream using future chaining and some under-the-hood type conversions.

PhucVH888 · 2016-05-16T13:47:26Z

I have been debugging the functional version, the bug is related to reference counter in future chaining.

After discussing with @albertnetymk , acquire_future_value(ctx, fut); is needed to get more counter. The issue is reported at #412 .

But the commit includes the unofficial fix of #412 here https://github.com/PhucVH888/encore/tree/tem/functional-stream-chain .

def chainR(sa:Stream A, f:A->B) : Stream B {
  let
    futa = embed (Fut Scons<A>) (future_t*)#{sa}; end
    sb   = embed (Stream B) (stream_t*)stream_mk(encore_ctx()); end
    fscons = \(scons:Scons<A>) ->
      if (embed bool scons_eos(encore_ctx(),(struct scons*)#{scons}); end) then
        embed Scons<B> (void*)scons_end(encore_ctx()); end
      else
        let va = embed A scons_element(encore_ctx(),(struct scons*)#{scons}).i; end in
        let nexta = embed Stream A scons_next(encore_ctx(),(struct scons*)#{scons}); end in
        let nextb = chain(nexta,f)
        in
        let currb = sb in
        let vb = f(va) in
          embed Scons<B>
            struct scons* scons = scons_put_fut(encore_ctx(),#{nextb},#{currb},(encore_arg_t) {.d =#{vb}},ENCORE_PRIMITIVE);
            (void*)scons;
          end
    futb = futa ~~> fscons
  in (embed Stream B (stream_t*)#{futb}; end)
}

supercooldave · 2016-05-16T14:10:39Z

In the code snippet above, sb should be defined within the fscons function.
Also, why is the function chainR calling function chain and not itself recursively?

PhucVH888 · 2016-05-16T14:25:19Z

I am very sorry for my premature code. The revised version still raises the same error as I posted in previous comment :(

TobiasWrigstad · 2016-05-16T14:25:55Z

@PhucVH888
It is possible — I'd say reasonable and encouraged — to write

let 
   x = …
   y = … 
   …
   z = …
in {
… 
}

rather than:

let x = … in 
let y = … in 
… 
let z = … in {
…
}

EliasC · 2016-05-16T14:27:59Z

@TobiasWrigstad: There is also the midway:

{
  let x = ...;
  let y = ...;
  ...
  let z = ...;
  ...
}

(note the lack of in and additional nested scopes)

PhucVH888 · 2016-05-16T14:29:18Z

@TobiasWrigstad Sometimes the variable x or z is not recognized by the compiler use of undeclared identifier '_enc__closure_va'; did you mean '_enc__closure_chain'?
That's why I have to use

let .. = ... in

But I will try with your solution or at least the midway solution.

TobiasWrigstad · 2016-05-16T14:33:12Z

@EliasC True!

But I prefer the let … in … as we're going to deprecate the single-line lets!

TobiasWrigstad · 2016-05-16T14:34:23Z

@PhucVH888
It seems you're using let … in rather than semi-colon ; for sequencing.

EliasC · 2016-05-16T14:34:40Z

as we're going to deprecate the single-line lets

You have given me permission to reimplement a similar syntactic sugar on top of the new syntax :)

TobiasWrigstad · 2016-05-16T14:42:04Z

Just as long as that syntax is var x = … (etc.)

I think?

EliasC · 2016-05-16T14:42:37Z

@TobiasWrigstad: +1

kikofernandez · 2016-05-17T09:16:56Z

bundles/prototype/StreamChain/StreamChain.enc

+typedef A = int
+typedef B = real
+
+passive class Scons<t>{}


@albertnetymk @EliasC I believe that if we have a parametric type that is not assigned to a field, then there won't be any generated tracing function for the parametric type t. I believe that, in practice, this means that an Scons<a> (or Scons<String>) is not tracing the parametric type (or the String). This could explain some of the GC crashes that you see @PhucVH888. Could @albertnetymk or @EliasC confirm this thought?

If there is a "hidden" field in the class, i.e. if the type is used to store some embedded thing, this will not be traced. It could definitely explain some crashes!

I am not sure I am fully following. A small failing example?

the example is compiling @PhucVH888's code and realising that whatever he puts in the Scons class is not going to be traced. You should copy paste the Encore code of this PR, generate C code and look at the Scons.encore.c file. The Scons generated class has an empty trace function. If @PhucVH888 puts something in the Scons, it won't be traced

This raises an issue. How does one talk about C data structures in Encore without messing up things like tracing.

Phuc's code manipulates a C data structure called scons by creating an class SCons to mask the underlying C pointer and allow it to be manipulated in Encore. But this seems to be the cause of the problem.

PhucVH888 · 2016-05-17T21:20:14Z

I have fixed the embedded type issue with embedded code to temporarily workaround. In addition, the commit is also passed all the tests with different sizes of input (5, 5.000, 1.000.000, 4.000.000) as well as the repeated tests in 100.000 times with 5.000 elements.

kikofernandez · 2016-05-17T21:21:18Z

bundles/prototype/StreamChain/StreamChain.enc

+          scons;
+        end;
+      }
+    -- futb = futa ~~> fscons


remove comment

supercooldave · 2016-05-18T05:26:31Z

The best thing is to redo this pull request after rebasing.

supercooldave · 2016-05-18T05:28:46Z

In addition, all the testing code should be removed from the bundle implementing stream chaining, and put into a separate file.

PhucVH888 · 2016-05-18T08:34:53Z

The commit has reflected all the comments aforementioned.

supercooldave · 2016-05-18T10:57:19Z

src/runtime/stream/stream.c

+  ctx = pony_ctx();
+  struct scons *scons = scons_mk(ctx, NULL);
+  scons->eos = true;
+  // scons->next = NULL;


This comment can be removed.

supercooldave · 2016-05-18T11:00:55Z

bundles/prototype/StreamChain/StreamChain.enc

+
+typedef A = int
+typedef B = real
+


Perhaps defining and using typedef SCons = embed struct scons* end would make the rest of the code a little more readable. (It would also test out typedefs a little more.)

If you make the chain function work on int -> int typed parameters rather than int -> real, we will be able to create pipelines of stream transformers etc.

PhucVH888 · 2016-05-18T12:35:04Z

Thanks @supercooldave . The code has been revised.

supercooldave · 2016-05-18T12:39:28Z

bundles/prototype/StreamChain/StreamChainTest.enc

+      print(get sa1);
+      sa1 = getNext sa1;
+    };
+    if eos sa1 then print "EOS";


If statement is not required.

albertnetymk · 2016-05-18T12:42:38Z

In building, the compiler complains scons_mk being declared as non-static, but defined as static. Am I the only one failing to build this PR?

Edit: How come CI builds?

albertnetymk · 2016-05-18T12:47:46Z

After removing static, PR builds. Running StreamChainTest.enc causes assertion failure in debug build.

PhucVH888 · 2016-05-18T13:26:52Z

@albertnetymk : It seems that did not happen to me. After clean up my branch, recompiling Encore shows some warnings such as getcontext, makecontext, struct VALUE_PARs, ...
Does anyone get the same warning as Albert's?

kikofernandez · 2016-05-18T13:30:07Z

It's normal to have the warnings related to getcontext, makecontext and VALUE_PARs. Some of then are related to using a deprecated library (ucontext.h) and the other related to 'abusing' the notation of anonymous structs.

supercooldave · 2016-05-18T20:06:52Z

This fails for me with streams of 5000 elements.
Error is
dave$ ./StreamChainTest Output stream: 0 Segmentation fault: 11

Running with Encore compiled in debug mode results in:
Output stream: Assertion failed: (obj), function gc_sendobject, file libponyrt/gc/gc.c, line 165. Abort trap: 6

I haven't investigated further.

PhucVH888 · 2016-05-18T20:48:30Z

I found the reason. I would like to take it back, there is not any problem with typedef.
After replacing Scons with embed and testing further with 1M elements, then it crashed.

It is because of the printing method call si.printSI() which is defined in active class.
But if we replace it with the while loop as previous then it's fine.

PhucVH888 · 2016-05-19T05:52:56Z

The commit has been tested with 5.000 elements and repeated tests in 100.000 times.

supercooldave · 2016-05-19T07:12:25Z

The fact that updates to this were made that failed the tests suggests that a better testing strategy is required. Simply having a single file that you modify to test different cases is insufficient. For extended versions of this code, please improve how you do the testing.

* Re-add stream chain * Add pwd to makefile to test * Fixed c-method name clashes * Remove an unnecessary folder in bundles/prototype * Revise StreamChain with polymorphic type * Fix import StreamChain library's path * Move streamIO to stream test folder

TobiasWrigstad added the ready for review label May 16, 2016

supercooldave reviewed May 16, 2016
View reviewed changes

PhucVH888 added in progress and removed ready for review labels May 16, 2016

kikofernandez reviewed May 17, 2016
View reviewed changes

PhucVH888 added ready for review and removed in progress labels May 17, 2016

kikofernandez reviewed May 17, 2016
View reviewed changes

bundles/prototype/StreamChain/StreamChain.enc

scons;

end;

}

-- futb = futa ~~> fscons

Copy link

Contributor

kikofernandez May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

Added functional stream chaining without using ~~>

a3aa858

supercooldave reviewed May 18, 2016
View reviewed changes

Added typedef Scons, revised chain::int->int, took eos out of loop

63f47c6

supercooldave reviewed May 18, 2016
View reviewed changes

Cleaned up code. Ready to merge.

c3a7938

Removed printSI(), added while loop to print results

e80853d

Fixed. Revised n from 5K to 5 elements.

e3b9fa0

supercooldave merged commit 2aaf7d1 into parapluu:development May 19, 2016

supercooldave removed the ready for review label May 19, 2016

PhucVH888 deleted the features/stream-chain branch September 13, 2016 09:20

PhucVH888 mentioned this pull request Sep 29, 2016

Re-add stream chain #416 #571

Merged

Stream chaining with encore primitive type #416

Stream chaining with encore primitive type #416

Conversation

PhucVH888 commented May 16, 2016

supercooldave commented May 16, 2016

Choose a reason for hiding this comment

supercooldave May 16, 2016 • edited Loading

Choose a reason for hiding this comment

PhucVH888 commented May 16, 2016 • edited Loading

supercooldave commented May 16, 2016

PhucVH888 commented May 16, 2016

TobiasWrigstad commented May 16, 2016 • edited Loading

EliasC commented May 16, 2016 • edited Loading

PhucVH888 commented May 16, 2016 • edited Loading

TobiasWrigstad commented May 16, 2016

TobiasWrigstad commented May 16, 2016

EliasC commented May 16, 2016

TobiasWrigstad commented May 16, 2016 • edited Loading

EliasC commented May 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikofernandez May 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhucVH888 commented May 17, 2016

Choose a reason for hiding this comment

supercooldave commented May 18, 2016

supercooldave commented May 18, 2016 • edited Loading

PhucVH888 commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhucVH888 commented May 18, 2016

Choose a reason for hiding this comment

albertnetymk commented May 18, 2016 • edited Loading

albertnetymk commented May 18, 2016

PhucVH888 commented May 18, 2016

kikofernandez commented May 18, 2016

supercooldave commented May 18, 2016

PhucVH888 commented May 18, 2016 • edited Loading

PhucVH888 commented May 19, 2016

supercooldave commented May 19, 2016

supercooldave May 16, 2016 •

edited

Loading

PhucVH888 commented May 16, 2016 •

edited

Loading

TobiasWrigstad commented May 16, 2016 •

edited

Loading

EliasC commented May 16, 2016 •

edited

Loading

PhucVH888 commented May 16, 2016 •

edited

Loading

TobiasWrigstad commented May 16, 2016 •

edited

Loading

kikofernandez May 17, 2016 •

edited

Loading

supercooldave commented May 18, 2016 •

edited

Loading

albertnetymk commented May 18, 2016 •

edited

Loading

PhucVH888 commented May 18, 2016 •

edited

Loading