[PREVIEW] Traversal oriented layout #22264

drewhk · 2017-02-06T09:41:20Z

No description provided.

akka-ci · 2017-02-06T09:46:37Z

Test FAILed.

drewhk · 2017-02-06T09:49:12Z

The best conflicts are those where you have already deleted the file in conflict ;)

patriknw · 2017-02-24T06:52:36Z

I don't think we had a specific ticket for this so I added #22387

patriknw · 2017-02-24T06:52:48Z

Refs #22387

akka-ci · 2017-03-01T09:46:45Z

Test FAILed.

akka-ci · 2017-03-01T12:46:32Z

Test FAILed.

akka-ci · 2017-03-01T14:41:40Z

Test FAILed.

akka-ci · 2017-03-01T15:43:07Z

Test FAILed.

drewhk · 2017-03-02T09:43:57Z

akka-stream-tests/src/test/scala/akka/stream/FusingSpec.scala

@@ -3,14 +3,8 @@
 */
 package akka.stream


Investigate what can be salvaged from this Spec. see #22434

drewhk · 2017-03-02T09:44:57Z

akka-stream-tests/src/test/scala/akka/stream/impl/TraversalBuilderSpec.scala

+      ))
+    }
+
+    //TODO: Dummy test cases just for smoke-testing. Should be removed.


Remove test cases below, they were used as quick smoke tests. No longer necessary.

drewhk · 2017-03-02T09:49:20Z

akka-stream-tests/src/test/scala/akka/stream/scaladsl/FlowSpec.scala

-
-    Flow.fromSinkAndSource(Sink.fromSubscriber(subscriber), Source.fromPublisher(publisher))
-  })
+  // TODO: Reenable these tests


ticket for this: #22435

drewhk · 2017-03-02T09:52:18Z

akka-stream/src/main/boilerplate/akka/stream/scaladsl/GraphApply.scala.template


    override def named(name: String): Graph[S, Mat] = addAttributes(Attributes.name(name))
  }
 }

-/**


This was not used anywhere actually.

It was introduced for GearPump in drewhk@1d692da
Perhaps obsolete now?

I am not sure, (I don't know what this was used for), but it will not work with the new layout this way. This can only be fixed I think if I understand what is the use case.

let's leave it out, everything is different anyway

drewhk · 2017-03-02T09:54:20Z

akka-stream/src/main/scala/akka/stream/Fusing.scala

- * attribute can be used to declare subgraph boundaries across which the graph
- * shall not be fused.
- */
-object Fusing {


This had to be removed because the FusedGraph and StructuralInfoModule types no longer exist and emulating them seems to be very hard. See: #22431

There is no point in pre-fusing anymore, it won't be faster.

drewhk · 2017-03-02T10:14:14Z

akka-stream/src/main/scala/akka/stream/impl/fusing/GraphStages.scala

-final case class GraphStageModule(
-  shape:      Shape,
+// TODO: Fix variance issues
+final case class GraphStageModule[+S <: Shape @uncheckedVariance, +M](


someone can maybe tell me why this does not work anymore. Or is this the right way to fix it.

drewhk · 2017-03-02T10:14:37Z

akka-stream/src/main/scala/akka/stream/impl/fusing/GraphStages.scala

    if (attributes ne this.attributes) new GraphStageModule(shape, attributes, stage)
    else this

+  override private[stream] def traversalBuilder = LinearTraversalBuilder.fromModule(this)


drewhk · 2017-03-02T10:14:47Z

akka-stream/src/main/scala/akka/stream/impl/fusing/GraphStages.scala

-   *
-   * This source is not reusable, it is only created internally.
-   */
-  final class MaterializedValueSource[T](val computation: MaterializedValueNode, val out: Outlet[T]) extends GraphStage[SourceShape[T]] {


gone for good!

drewhk · 2017-03-02T10:15:11Z

akka-stream/src/main/scala/akka/stream/impl/fusing/IteratorInterpreter.scala

-    interpreter.attachDownstreamBoundary(connections(length), downstream)
-    interpreter.init(null)
+
+    // TODO: Fix this (assembly is gone)


drewhk · 2017-03-02T10:15:29Z

akka-stream/src/main/scala/akka/stream/impl/fusing/StreamOfStreams.scala

@@ -643,29 +643,6 @@ final class SubSink[T](name: String, externalCallback: ActorSubscriberMessage
  override def toString: String = name
 }

-object SubSource {


this was not used anywhere

It was in Akka HTTP actually

Not needed anymore, I replaced it there.

patriknw

mostly small comments, feel free to create tickets if something is not quick fix

patriknw · 2017-03-06T06:49:11Z

akka-stream/src/main/boilerplate/akka/stream/scaladsl/GraphApply.scala.template


    override def named(name: String): Graph[S, Mat] = addAttributes(Attributes.name(name))
  }
 }

-/**


It was introduced for GearPump in drewhk@1d692da
Perhaps obsolete now?

patriknw · 2017-03-06T06:57:59Z

akka-stream/src/main/scala/akka/stream/impl/PhasedFusingActorMaterializer.scala

+}
+
+private case class SegmentInfo(
+  globalislandOffset: Int, // The island to which the segment belongs


camel case on island?
globalIslandOffset

patriknw · 2017-03-06T06:59:33Z

akka-stream/src/main/scala/akka/stream/impl/PhasedFusingActorMaterializer.scala

+}
+
+private case class ForwardWire(
+  islandGlobalOffset: Int,


globalislandOffset vs islandGlobalOffset?

right, these names should be consistent. Will fix.

patriknw · 2017-03-06T07:05:16Z

akka-stream/src/main/scala/akka/stream/impl/PhasedFusingActorMaterializer.scala

+import scala.concurrent.{ ExecutionContextExecutor, Future }
+import scala.util.Random
+
+object PhasedFusingActorMaterializer {


In Banff you showed a nice sketch of what islands and segments are. Could you describe that here somewhere?

Yes, I will add internal documentation after things are fixed. I create a ticket.

patriknw · 2017-03-06T07:12:29Z

akka-stream/src/main/scala/akka/stream/impl/PhasedFusingActorMaterializer.scala

+
+}
+
+case class PhasedFusingActorMaterializer(


any reason this is a case class?
final ?

Yes it should be final. Probably does not need to be a case class either.

patriknw · 2017-03-06T08:01:19Z

akka-stream/src/main/scala/akka/stream/impl/TraversalBuilder.scala

+ * in a fixed location, therefore the last step of the Traversal might need to be changed in those cases from the
+ * -1 relative offset to something else (see rewireLastOutTo).
+ */
+final case class LinearTraversalBuilder(


make everything in impl INTERNAL API, unless intended otherwise

patriknw · 2017-03-06T08:08:00Z

akka-stream/src/main/scala/akka/stream/impl/TraversalBuilder.scala

+    val builderKey = new BuilderKey
+
+    val newBuildSteps =
+      if (combineMat == Keep.left) {


We convert those earlier in the javadsl so we don't have to care about javadsl.
Perhaps worth a comment.

patriknw · 2017-03-06T08:10:51Z

akka-stream/src/main/scala/akka/stream/impl/TraversalBuilder.scala

+        newInOffsets = newInOffsets.updated(in, inSlots + submodule.offsetOf(in.mappedTo))
+      }
+
+      val outIterator = shape.outlets.iterator


if there are cases where inlets or outlets can be empty it might be worth checking nonEmpty before creating the iterator, note that even Nil.iterator creates a new instance

good one, yes.

patriknw · 2017-03-06T08:18:39Z

akka-stream/src/main/scala/akka/stream/scaladsl/Source.scala

@@ -27,7 +28,10 @@ import scala.compat.java8.FutureConverters._
 * an “atomic” source, e.g. from a collection or a file. Materialization turns a Source into
 * a Reactive Streams `Publisher` (at least conceptually).
 */
-final class Source[+Out, +Mat](override val module: Module)
+final class Source[+Out, +Mat](
+  override val traversalBuilder: LinearTraversalBuilder,


what should be public? exposing impl as public seems wrong

Hm, strange why it was public before. Maybe Gearpump needed it? We can make it hidden now as a custom phase will no longer need to know about the internal representation. Thanks for pointing out.

patriknw · 2017-03-06T08:20:21Z

akka-stream/src/main/scala/akka/stream/scaladsl/Source.scala

-        this.mapMaterializedValue((_) ⇒ NotUsed).asInstanceOf[Source[T, Mat3]]
-      else
-        this.mapMaterializedValue(combine(_, NotUsed.asInstanceOf[Mat2])).asInstanceOf[Source[T, Mat3]]
+    if (flow.traversalBuilder eq Flow.identityTraversalBuilder) {


what happened with the Keep optimizations here?

They are implemented in the traversal now.

patriknw · 2017-03-06T13:05:54Z

This PR is only intended for reviews of the initial commit. Development is going on in wip-traversal-oriented-layout-drewhk and new PRs will be opened to that branch.

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Feb 6, 2017

patriknw mentioned this pull request Feb 7, 2017

Akka Sprint Plan 2017-02-06 akka/akka-meta#42

Closed

jrudolph mentioned this pull request Feb 20, 2017

encodeResponse with gzip is very slow akka/akka-http#841

Closed

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 1, 2017

New materializer and layout

ba63c7a

drewhk force-pushed the wip-traversal-oriented-layout-drewhk branch from 53192b2 to ba63c7a Compare March 1, 2017 15:27

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 1, 2017

akka-ci removed the validating PR is currently being validated by Jenkins label Mar 1, 2017

drewhk commented Mar 2, 2017

View reviewed changes

patriknw reviewed Mar 6, 2017

View reviewed changes

drewhk mentioned this pull request Mar 6, 2017

Review the materializer's and the GraphStagePhase's uses of arrays (ArrayList, ArrayDeque, etc) #22429

Closed

Horusiath mentioned this pull request Mar 8, 2017

Implement PhasedFusingActorMaterializer akkadotnet/akka.net#2552

Open

patriknw merged commit ba63c7a into akka:master Mar 10, 2017

[PREVIEW] Traversal oriented layout #22264

[PREVIEW] Traversal oriented layout #22264

Uh oh!

Conversation

drewhk commented Feb 6, 2017

Uh oh!

akka-ci commented Feb 6, 2017

Uh oh!

drewhk commented Feb 6, 2017

Uh oh!

patriknw commented Feb 24, 2017

Uh oh!

patriknw commented Feb 24, 2017

Uh oh!

akka-ci commented Mar 1, 2017

Uh oh!

akka-ci commented Mar 1, 2017

Uh oh!

akka-ci commented Mar 1, 2017

Uh oh!

akka-ci commented Mar 1, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

patriknw 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!

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!

Choose a reason for hiding this comment