Auto push full impl #537

FelonEkonom · 2023-03-13T16:59:14Z

Related Jira ticket: https://membraneframework.atlassian.net/browse/MC-163

mat-hek · 2023-05-16T11:25:42Z

lib/membrane/core/element/demand_counter/distributed_flow_mode.ex

+defmodule Membrane.Core.Element.DemandCounter.DistributedFlowMode do
+  @moduledoc false
+
+  alias Membrane.Core.Element.DemandCounter.DistributedAtomic
+  alias Membrane.Core.Element.EffectiveFlowController
+
+  @type t :: DistributedAtomic.t()
+  @type flow_mode_value ::
+          EffectiveFlowController.effective_flow_control() | :to_be_resolved


Suggested change

defmodule Membrane.Core.Element.DemandCounter.DistributedFlowMode do

@moduledoc false

alias Membrane.Core.Element.DemandCounter.DistributedAtomic

alias Membrane.Core.Element.EffectiveFlowController

@type t :: DistributedAtomic.t()

@type flow_mode_value ::

EffectiveFlowController.effective_flow_control() | :to_be_resolved

defmodule Membrane.Core.Element.DemandCounter.DistributedFlowStatus do

@moduledoc false

alias Membrane.Core.Element.DemandCounter.DistributedAtomic

alias Membrane.Core.Element.EffectiveFlowController

@type t :: DistributedAtomic.t()

@type flow_control_status ::

{:resolved, EffectiveFlowController.effective_flow_control()} | :to_be_resolved

mat-hek · 2023-05-16T11:26:14Z

lib/membrane/core/element/demand_counter.ex

+            receiver_demand_unit: Membrane.Buffer.Metric.unit()
+          }
+
+  @type flow_mode :: DistributedFlowMode.flow_mode_value()


Suggested change

@type flow_mode :: DistributedFlowMode.flow_mode_value()

mat-hek · 2023-05-16T11:28:17Z

lib/membrane/core/element/demand_controller.ex

@@ -91,7 +89,9 @@ defmodule Membrane.Core.Element.DemandController do
  @spec decrease_demand_by_outgoing_buffers(Pad.ref(), [Buffer.t()], State.t()) :: State.t()
  def decrease_demand_by_outgoing_buffers(pad_ref, buffers, state) do
    pad_data = PadModel.get_data!(state, pad_ref)
-    buffers_size = Buffer.Metric.from_unit(pad_data.other_demand_unit).buffers_size(buffers)
+
+    demand_unit = pad_data.demand_unit || pad_data.other_demand_unit || :buffers


demand unit should be resolved when the pad is linked

Yes, but this alternative matters when output pad has :push flow control

So maybe we should calculate the demand unit no matter the flow control (since the demand counter is always there)

IMO it does not look good to have non-nil demand_unit when flow_control is :push. I have changed this line to make it explicit, that we take other_demand_unit into account only when flow_control is :push and pad_data.demand_unit when flow_control is :pull or :auto

IMO it does not look good to have non-nil demand_unit when flow_control is :push.

IMO it looks even worse to have demand_counter and not have demand_unit there 🤔 And I see no point in resolving it each time when we can do it once.

Maybe the field should be buffer_size_unit, not demand_unit :D

lib/membrane/core/element/demand_counter.ex

lib/membrane/element/pad_data.ex

mat-hek · 2023-05-17T13:30:49Z

lib/membrane/core/element/pad_controller.ex

@@ -241,6 +244,18 @@ defmodule Membrane.Core.Element.PadController do
    end
  end

+  @spec handle_input_pad_added(Pad.ref(), State.t()) :: State.t()
+  def handle_input_pad_added(pad_ref, state) do


It's only called once, in the same module. Remove it and put the contents where it's called

mat-hek · 2023-05-19T09:34:32Z

lib/membrane/core/element/demand_counter/distributed_atomic.ex

@@ -19,7 +19,7 @@ defmodule Membrane.Core.Element.DemandCounter.DistributedAtomic do
  @spec new(integer() | nil) :: t
  def new(initial_value \\ nil) do
    atomic_ref = :atomics.new(1, [])
-    {:ok, worker} = Worker.start_link()
+    {:ok, worker} = Worker.start_link(self())


let's start it under the supervisor

mat-hek · 2023-05-19T09:39:47Z

lib/membrane/core/element/effective_flow_controller.ex

         %{effective_flow_control: effective_flow_control} = state
       ),
       do: state

-  defp set_effective_flow_control(new_effective_flow_control, state) do
+  defp set_effective_flow_control(new_effective_flow_control, last_changed_pad, state) do


Suggested change

defp set_effective_flow_control(new_effective_flow_control, last_changed_pad, state) do

defp set_effective_flow_control(new_effective_flow_control, triggerer_pad, state) do

mat-hek · 2023-05-19T09:40:03Z

lib/membrane/core/element/effective_flow_controller.ex

          [pad_data.other_ref, new_effective_flow_control]
        )

        state

+      {pad_ref, %{direction: :input} = pad_data}, state when last_changed_pad != nil ->


Suggested change

{pad_ref, %{direction: :input} = pad_data}, state when last_changed_pad != nil ->

{pad_ref, %{direction: :input} = pad_data}, state ->

lib/membrane/core/element/demand_counter.ex

lib/membrane/core/element/demand_counter/distributed_flow_status.ex

lib/membrane/core/element/demand_counter.ex

lib/membrane/core/element/demand_handler.ex

lib/membrane/core/element/effective_flow_controller.ex

mat-hek

🎉

FelonEkonom · 2023-05-22T12:14:46Z

🎉

awesome

varsill

There is one small comment concerning the docs, but generally speaking:
LGTM 🥇

varsill · 2023-05-22T12:31:40Z

lib/membrane/core/element/demand_controller.ex

@@ -1,7 +1,7 @@
 defmodule Membrane.Core.Element.DemandController do
  @moduledoc false

-  # Module handling changes in values of output pads demand counters
+  # Module handling changes in values of output pads demand atomics


Suggested change

# Module handling changes in values of output pads demand atomics

# Module handling changes in values of output pads atomic demand

To be fair, I believe it should be written that way, but I don't find it anyhow crucial :D

FelonEkonom added 14 commits February 27, 2023 15:03

Implement resolving auto pads mode

821f8c9

Refactor effective flow control related code

328837b

Rename file auto_demand_controller to effective_flow_control_controller

1d5f38b

Add simple test for effective flow control resoultion

490d651

Refactor & upgrade toilet

c291687

Fix credo issues

ba8bb81

Fill toilet always on sending buffers via push output pad

62315ba

Drain toilet only if it exists

7116b94

Fix credo issue

a156819

Rename :undefined to :not_resolved

eaab0bc

Implement comments from PR

a1dcfef

Merge remote-tracking branch 'origin/master' into auto-push

89e85bc

Fix dialyzer issues

012574b

Refactor Element test

e5c8305

FelonEkonom force-pushed the auto-push-full-impl branch from 17ac82e to 4af3609 Compare March 15, 2023 11:07

Implement full version of auto-push

70f1842

FelonEkonom force-pushed the auto-push-full-impl branch from 4af3609 to 70f1842 Compare March 15, 2023 11:13

FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Mar 15, 2023

Merge remote-tracking branch 'origin/master' into auto-push-full-impl

e819388

FelonEkonom force-pushed the auto-push-full-impl branch from 8d7e3d9 to e819388 Compare March 15, 2023 12:30

FelonEkonom marked this pull request as ready for review March 15, 2023 12:32

FelonEkonom requested a review from mat-hek as a code owner March 15, 2023 12:32

FelonEkonom requested a review from varsill March 15, 2023 12:33

FelonEkonom mentioned this pull request Mar 15, 2023

Implement auto push #533

Closed

Implement DemandCounter

38ff2a7

FelonEkonom force-pushed the auto-push-full-impl branch from 436a9be to 38ff2a7 Compare March 17, 2023 13:46

FelonEkonom added 2 commits March 17, 2023 17:11

wip

a95ee9a

wip

73820a5

FelonEkonom marked this pull request as draft March 20, 2023 12:28

wip

6801f3c

FelonEkonom requested a review from mat-hek May 16, 2023 11:14

FelonEkonom added 3 commits May 16, 2023 13:19

Merge remote-tracking branch 'origin/master' into auto-push-full-impl

0a88e25

Terminate distributed atomic worker after it's creator death

97d0b22

Update algorithm of changing flags in DemandCounter

47964a5

mat-hek requested changes May 17, 2023

View reviewed changes

FelonEkonom added 2 commits May 17, 2023 16:57

Implement suggestion from CR wip

27bc14c

Refactor getting demand unit in demand controller

e713e52

FelonEkonom requested a review from mat-hek May 18, 2023 13:05

Rename lacking_buffers_size to demand

1a1957a

FelonEkonom force-pushed the auto-push-full-impl branch from 27c0b66 to 1a1957a Compare May 18, 2023 13:06

mat-hek requested changes May 19, 2023

View reviewed changes

FelonEkonom added 4 commits May 19, 2023 14:47

Implement suggestion from CR

71ca3e7

Merge remote-tracking branch 'origin/master' into auto-push-full-impl

ae869b2

Fix Error log

7959a6a

Rename DemandCounter to AtomicDemand

812cf9c

varsill reviewed May 19, 2023

View reviewed changes

FelonEkonom added 6 commits May 19, 2023 16:36

Start AtomicDemand Worker under supervisor

0821fb5

Fix AtomicDemand tests wip

765adad

Sort aliases

70840cb

Fix tests failing because of changes in AtomicDemand

efc9edb

Implememnt suggestions from CR

f3ef798

Merge remote-tracking branch 'origin/master' into auto-push-full-impl

74a94e6

FelonEkonom requested review from varsill and mat-hek May 22, 2023 11:51

mat-hek approved these changes May 22, 2023

View reviewed changes

varsill approved these changes May 22, 2023

View reviewed changes

Fix typo in comment

3d8f81c

FelonEkonom merged commit 0328b57 into master May 22, 2023

FelonEkonom deleted the auto-push-full-impl branch May 22, 2023 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto push full impl #537

Auto push full impl #537

FelonEkonom commented Mar 13, 2023 •

edited

Loading

mat-hek May 16, 2023

mat-hek May 16, 2023

mat-hek May 16, 2023

FelonEkonom May 17, 2023 •

edited

Loading

mat-hek May 18, 2023

FelonEkonom May 18, 2023

mat-hek May 19, 2023

mat-hek May 17, 2023

mat-hek May 19, 2023

mat-hek May 19, 2023

mat-hek May 19, 2023

mat-hek left a comment

FelonEkonom commented May 22, 2023

varsill left a comment

varsill May 22, 2023

	defp set_effective_flow_control(new_effective_flow_control, last_changed_pad, state) do
	defp set_effective_flow_control(new_effective_flow_control, triggerer_pad, state) do

	{pad_ref, %{direction: :input} = pad_data}, state when last_changed_pad != nil ->
	{pad_ref, %{direction: :input} = pad_data}, state ->

	# Module handling changes in values of output pads demand atomics
	# Module handling changes in values of output pads atomic demand

Auto push full impl #537

Auto push full impl #537

Conversation

FelonEkonom commented Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek left a comment

Choose a reason for hiding this comment

FelonEkonom commented May 22, 2023

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom commented Mar 13, 2023 •

edited

Loading

FelonEkonom May 17, 2023 •

edited

Loading