Revert "Sort component state fields in the error logs in the order fr…

…om the most to the least important (#614)" This reverts commit 56ebe56.
membraneframework · Oct 12, 2023 · db4792f · db4792f
1 parent 4386843
commit db4792f
Show file tree

Hide file tree

Showing 10 changed files with 58 additions and 150 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -154,7 +154,6 @@ jobs:
           at: .
       - run: cp -r benchmark/ ~/benchmark_backup/
       - run: cp mix.exs ~/benchmark_backup/
-      - run: docker pull membraneframeworklabs/docker_membrane
       - run: docker run -e MIX_ENV=benchmark -v ./:/root/app -v ~/results:/root/results -w /root/app membraneframeworklabs/docker_membrane mix do deps.get, deps.compile --force --all, run benchmark/run.exs /root/results/feature_branch_results
       - run: git checkout -f master
       - run: cp  ~/benchmark_backup/mix.exs ~/app

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,7 +26,6 @@
  * Add child exit reason to the supervisor exit reason. [#595](https://github.com/membraneframework/membrane_core/pull/595)
  * Remove default implementation of `start_/2`, `start_link/2` and `terminate/2` in modules using `Membrane.Pipeline`. [#598](https://github.com/membraneframework/membrane_core/pull/598) 
  * Remove callback _Membrane.Element.WithInputPads.handle_buffers_batch/4_. [#601](https://github.com/membraneframework/membrane_core/pull/601)
- * Sort component state fields in the error logs in the order from the most to the least important. [#614](https://github.com/membraneframework/membrane_core/pull/614)
 
 ## 0.11.0
  * Separate element_name and pad arguments in handle_element_{start, end}_of_stream signature [#219](https://github.com/membraneframework/membrane_core/issues/219)

diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex
@@ -51,7 +51,6 @@ defmodule Membrane.Core.Bin.PadController do
       case PadModel.get_data(state, pad_ref) do
         {:error, :unknown_pad} ->
           init_pad_data(pad_ref, pad_info, state)
-          |> Map.update!(:pad_refs, &[pad_ref | &1])
 
         # This case is for pads that were instantiated before the external link request,
         # that is in the internal link request (see `handle_internal_link_request/4`).
@@ -282,10 +281,8 @@ defmodule Membrane.Core.Bin.PadController do
   @spec handle_unlink(Pad.ref(), Core.Bin.State.t()) :: Core.Bin.State.t()
   def handle_unlink(pad_ref, state) do
     with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do
-      {pad_data, state} =
-        maybe_handle_pad_removed(pad_ref, state)
-        |> Map.update!(:pad_refs, &List.delete(&1, pad_ref))
-        |> PadModel.pop_data!(pad_ref)
+      state = maybe_handle_pad_removed(pad_ref, state)
+      {pad_data, state} = PadModel.pop_data!(state, pad_ref)
 
       if pad_data.endpoint do
         Message.send(pad_data.endpoint.pid, :handle_unlink, pad_data.endpoint.pad_ref)

diff --git a/lib/membrane/core/bin/state.ex b/lib/membrane/core/bin/state.ex
@@ -8,7 +8,7 @@ defmodule Membrane.Core.Bin.State do
   use Bunch
   use Bunch.Access
 
-  alias Membrane.{Child, Clock, Pad, Sync}
+  alias Membrane.{Child, Clock, Sync}
   alias Membrane.Core.Child.PadModel
   alias Membrane.Core.Parent.ChildLifeController
   alias Membrane.Core.Parent.{ChildrenModel, CrashGroup, Link}
@@ -20,7 +20,6 @@ defmodule Membrane.Core.Bin.State do
           children: ChildrenModel.children(),
           subprocess_supervisor: pid(),
           name: Membrane.Bin.name() | nil,
-          pad_refs: [Pad.ref()],
           pads_info: PadModel.pads_info() | nil,
           pads_data: PadModel.pads_data() | nil,
           parent_pid: pid,
@@ -50,34 +49,23 @@ defmodule Membrane.Core.Bin.State do
           stalker: Membrane.Core.Stalker.t()
         }
 
-  # READ THIS BEFORE ADDING NEW FIELD!!!
-
-  # Fields of this structure will be inspected in the same order, in which they occur in the
-  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
-  # If you want to add a new field to the state, place it at the spot corresponding to its
-  # importance and possibly near other related fields. It is suggested, to keep `:pads_data`
-  # as the last item in the list, because sometimes it is so big, that everything after it
-  # might be truncated during the inspection.
-
-  defstruct module: nil,
-            name: nil,
-            parent_pid: nil,
-            playback: :stopped,
-            internal_state: nil,
-            pad_refs: [],
-            pads_info: nil,
-            children: %{},
-            links: %{},
-            crash_groups: %{},
-            pending_specs: %{},
-            synchronization: nil,
-            initialized?: false,
-            terminating?: false,
-            setup_incomplete?: false,
-            handling_action?: false,
-            stalker: nil,
-            resource_guard: nil,
-            subprocess_supervisor: nil,
-            children_log_metadata: [],
-            pads_data: nil
+  @enforce_keys [:module, :synchronization, :subprocess_supervisor, :resource_guard, :stalker]
+  defstruct @enforce_keys ++
+              [
+                internal_state: nil,
+                children: %{},
+                name: nil,
+                pads_info: nil,
+                pads_data: nil,
+                parent_pid: nil,
+                crash_groups: %{},
+                children_log_metadata: [],
+                links: %{},
+                pending_specs: %{},
+                playback: :stopped,
+                initialized?: false,
+                terminating?: false,
+                setup_incomplete?: false,
+                handling_action?: false
+              ]
 end
diff --git a/lib/membrane/core/child/pad_spec_handler.ex b/lib/membrane/core/child/pad_spec_handler.ex
@@ -21,8 +21,7 @@ defmodule Membrane.Core.Child.PadSpecHandler do
       | pads_info:
           get_pads(state)
           |> Map.new(),
-        pads_data: %{},
-        pad_refs: []
+        pads_data: %{}
     }
   end
 

diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex
@@ -228,10 +228,7 @@ defmodule Membrane.Core.Element.PadController do
       state = generate_eos_if_needed(pad_ref, state)
       state = maybe_handle_pad_removed(pad_ref, state)
       state = remove_pad_associations(pad_ref, state)
-
-      {pad_data, state} =
-        Map.update!(state, :pad_refs, &List.delete(&1, pad_ref))
-        |> PadModel.pop_data!(pad_ref)
+      {pad_data, state} = PadModel.pop_data!(state, pad_ref)
 
       with %{direction: :input, flow_control: :auto, other_effective_flow_control: :pull} <-
              pad_data do
@@ -317,10 +314,7 @@ defmodule Membrane.Core.Element.PadController do
       |> merge_pad_mode_data(endpoint.pad_props, other_pad_info, state)
       |> then(&struct!(Membrane.Element.PadData, &1))
 
-    state =
-      state
-      |> put_in([:pads_data, endpoint.pad_ref], pad_data)
-      |> Map.update!(:pad_refs, &[endpoint.pad_ref | &1])
+    state = put_in(state, [:pads_data, endpoint.pad_ref], pad_data)
 
     :ok =
       AtomicDemand.set_sender_status(

diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex
@@ -19,7 +19,6 @@ defmodule Membrane.Core.Element.State do
           type: Element.type(),
           name: Element.name(),
           internal_state: Element.state() | nil,
-          pad_refs: [Pad.ref()] | nil,
           pads_info: PadModel.pads_info() | nil,
           pads_data: PadModel.pads_data() | nil,
           parent_pid: pid,
@@ -46,39 +45,29 @@ defmodule Membrane.Core.Element.State do
           stalker: Membrane.Core.Stalker.t()
         }
 
-  # READ THIS BEFORE ADDING NEW FIELD!!!
-
-  # Fields of this structure will be inspected in the same order, in which they occur in the
-  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
-  # If you want to add a new field to the state, place it at the spot corresponding to its
-  # importance and possibly near other related fields. It is suggested, to keep `:pads_data`
-  # as the last item in the list, because sometimes it is so big, that everything after it
-  # might be truncated during the inspection.
-
   defstruct [
     :module,
-    :name,
-    :parent_pid,
-    :playback,
     :type,
+    :name,
     :internal_state,
-    :pad_refs,
     :pads_info,
-    :synchronization,
+    :pads_data,
+    :parent_pid,
+    :supplying_demand?,
     :delayed_demands,
-    :effective_flow_control,
+    :handle_demand_loop_counter,
+    :synchronization,
+    :demand_size,
     :initialized?,
+    :playback,
+    :playback_queue,
+    :resource_guard,
+    :subprocess_supervisor,
     :terminating?,
     :setup_incomplete?,
-    :supplying_demand?,
+    :effective_flow_control,
     :handling_action?,
-    :stalker,
-    :resource_guard,
-    :subprocess_supervisor,
-    :handle_demand_loop_counter,
-    :demand_size,
     :pads_to_snapshot,
-    :playback_queue,
-    :pads_data
+    :stalker
   ]
 end
diff --git a/lib/membrane/core/inspect.ex b/lib/membrane/core/inspect.ex
diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex
@@ -9,17 +9,15 @@ defmodule Membrane.Core.Pipeline.State do
   use Bunch.Access
 
   alias Membrane.Child
-  alias Membrane.Core.Parent.{ChildLifeController, ChildrenModel, CrashGroup, Link}
+  alias Membrane.Core.Parent.{ChildrenModel, CrashGroup, Link}
   alias Membrane.Core.Timer
 
   @type t :: %__MODULE__{
-          module: module,
-          playback: Membrane.Playback.t(),
           internal_state: Membrane.Pipeline.state() | nil,
+          module: module,
           children: ChildrenModel.children(),
-          links: %{Link.id() => Link.t()},
           crash_groups: %{CrashGroup.name() => CrashGroup.t()},
-          pending_specs: ChildLifeController.pending_specs(),
+          links: %{Link.id() => Link.t()},
           synchronization: %{
             timers: %{Timer.id() => Timer.t()},
             clock_provider: %{
@@ -29,35 +27,27 @@ defmodule Membrane.Core.Pipeline.State do
             },
             clock_proxy: Membrane.Clock.t()
           },
+          playback: Membrane.Playback.t(),
           initialized?: boolean(),
           terminating?: boolean(),
           resource_guard: Membrane.ResourceGuard.t(),
           setup_incomplete?: boolean(),
           handling_action?: boolean(),
-          stalker: Membrane.Core.Stalker.t(),
-          subprocess_supervisor: pid()
+          stalker: Membrane.Core.Stalker.t()
         }
 
-  # READ THIS BEFORE ADDING NEW FIELD!!!
-
-  # Fields of this structure will be inspected in the same order, in which they occur in the
-  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
-  # If you want to add a new field to the state, place it at the spot corresponding to its
-  # importance and possibly near other related fields.
-
-  defstruct module: nil,
-            playback: :stopped,
-            internal_state: nil,
-            children: %{},
-            links: %{},
-            crash_groups: %{},
-            pending_specs: %{},
-            synchronization: nil,
-            initialized?: false,
-            terminating?: false,
-            setup_incomplete?: false,
-            handling_action?: false,
-            stalker: nil,
-            resource_guard: nil,
-            subprocess_supervisor: nil
+  @enforce_keys [:module, :synchronization, :subprocess_supervisor, :resource_guard, :stalker]
+  defstruct @enforce_keys ++
+              [
+                internal_state: nil,
+                children: %{},
+                crash_groups: %{},
+                links: %{},
+                pending_specs: %{},
+                playback: :stopped,
+                initialized?: false,
+                terminating?: false,
+                setup_incomplete?: false,
+                handling_action?: false
+              ]
 end
diff --git a/test/membrane/core/element/pad_controller_test.exs b/test/membrane/core/element/pad_controller_test.exs
@@ -61,9 +61,7 @@ defmodule Membrane.Core.Element.PadControllerTest do
                  state
                )
 
-      assert Map.drop(new_state, [:pads_data, :pad_refs]) ==
-               Map.drop(state, [:pads_data, :pad_refs])
-
+      assert %{new_state | pads_data: nil} == %{state | pads_data: nil}
       assert PadModel.assert_instance(new_state, :input) == :ok
     end