From bfac486df51727fbe7399c871144c1db7a2b7089 Mon Sep 17 00:00:00 2001 From: Jamil Date: Thu, 18 Sep 2025 14:37:37 -0400 Subject: [PATCH] refactor(portal): use list comprehensions in cache (#10376) Elixir's [list comprehensions](https://hexdocs.pm/elixir/comprehensions.html) are more concise and [often faster](https://stackoverflow.com/questions/55038704/elixir-enum-map-vs-for-comprehension) (~2x) than using multiple Enum.filter and Enum.map calls. Since I was in these modules debugging possible a race condition for #10375, I decided to go ahead and update some of these hot functions to use the more modern approach. --------- Signed-off-by: Jamil Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- elixir/apps/domain/lib/domain/cache/client.ex | 76 +++++++------------ .../apps/domain/lib/domain/cache/gateway.ex | 22 +++--- 2 files changed, 37 insertions(+), 61 deletions(-) diff --git a/elixir/apps/domain/lib/domain/cache/client.ex b/elixir/apps/domain/lib/domain/cache/client.ex index c0338e4dd..e86720277 100644 --- a/elixir/apps/domain/lib/domain/cache/client.ex +++ b/elixir/apps/domain/lib/domain/cache/client.ex @@ -96,9 +96,7 @@ defmodule Domain.Cache.Client do resource = Enum.find(cache.connectable_resources, :not_found, fn r -> r.id == rid_bytes end) policy = - cache.policies - |> Enum.filter(fn {_id, policy} -> policy.resource_id == rid_bytes end) - |> Enum.map(fn {_id, policy} -> policy end) + for({_id, %{resource_id: ^rid_bytes} = p} <- cache.policies, do: p) |> Policies.longest_conforming_policy_for_client(client, subject.expires_at) with %Cache.Cacheable.Resource{} <- resource, @@ -150,7 +148,7 @@ defmodule Domain.Cache.Client do end def recompute_connectable_resources(cache, client, opts \\ []) do - {toggle, _opts} = Keyword.pop(opts, :toggle) + {toggle, _opts} = Keyword.pop(opts, :toggle, false) connectable_resources = cache.policies @@ -163,19 +161,15 @@ defmodule Domain.Cache.Client do # connlib can handle all resource attribute changes except for changing sites, so we can omit the deleted IDs # of added resources since they'll be updated gracefully. - removed = - (cache.connectable_resources -- connectable_resources) - |> Enum.filter(fn r -> - if toggle do - r - else - r.id not in added_ids - end - end) + removed_ids = + for r <- cache.connectable_resources -- connectable_resources, + toggle or r.id not in added_ids do + load!(r.id) + end cache = %{cache | connectable_resources: connectable_resources} - {:ok, added, Enum.map(removed, & &1.id) |> Enum.map(&load!/1), cache} + {:ok, added, removed_ids, cache} end @doc """ @@ -230,23 +224,17 @@ defmodule Domain.Cache.Client do gid_bytes = dump!(membership.group_id) updated_policies = - cache.policies - |> Enum.reject(fn {_id, p} -> p.actor_group_id == gid_bytes end) - |> Enum.into(%{}) + for {id, p} <- cache.policies, p.actor_group_id != gid_bytes, do: {id, p}, into: %{} # Only remove resources that have no remaining policies remaining_resource_ids = - updated_policies - |> Enum.map(fn {_id, p} -> p.resource_id end) - |> Enum.uniq() - |> MapSet.new() + for {_id, p} <- updated_policies, do: p.resource_id, into: MapSet.new() updated_resources = - cache.resources - |> Enum.filter(fn {rid_bytes, _resource} -> - MapSet.member?(remaining_resource_ids, rid_bytes) - end) - |> Enum.into(%{}) + for {rid_bytes, resource} <- cache.resources, + MapSet.member?(remaining_resource_ids, rid_bytes), + do: {rid_bytes, resource}, + into: %{} updated_memberships = cache.memberships @@ -274,21 +262,14 @@ defmodule Domain.Cache.Client do # Get updated resources resources = - cache.resources - |> Enum.map(fn {id, resource} -> - # Replace old group with new + for {id, resource} <- cache.resources, into: %{} do gateway_groups = - Enum.map(resource.gateway_groups, fn gg -> - if gg.id == group.id do - group - else - gg - end - end) + for gg <- resource.gateway_groups do + if gg.id == group.id, do: group, else: gg + end {id, %{resource | gateway_groups: gateway_groups}} - end) - |> Enum.into(%{}) + end cache = %{cache | resources: resources} @@ -522,11 +503,9 @@ defmodule Domain.Cache.Client do end) memberships = - Actors.all_memberships_for_actor_id!(client.actor_id) - |> Enum.map(fn membership -> - {dump!(membership.group_id), dump!(membership.id)} - end) - |> Enum.into(%{}) + for memberships <- Actors.all_memberships_for_actor_id!(client.actor_id), + do: {dump!(memberships.group_id), dump!(memberships.id)}, + into: %{} cache |> Map.put(:memberships, memberships) @@ -535,11 +514,12 @@ defmodule Domain.Cache.Client do end defp adapted_resources(conforming_resource_ids, resources, client) do - conforming_resource_ids - |> Enum.map(fn id -> Map.get(resources, id) end) - |> Enum.map(fn r -> adapt(r, client) end) - |> Enum.reject(fn r -> is_nil(r) end) - |> Enum.filter(fn r -> r.gateway_groups != [] end) + for id <- conforming_resource_ids, + adapted_resource = Map.get(resources, id) |> adapt(client), + not is_nil(adapted_resource), + adapted_resource.gateway_groups != [] do + adapted_resource + end end defp conforming_resource_ids(policies, client) when is_map(policies) do diff --git a/elixir/apps/domain/lib/domain/cache/gateway.ex b/elixir/apps/domain/lib/domain/cache/gateway.ex index 30636b633..a4c1c5a4b 100644 --- a/elixir/apps/domain/lib/domain/cache/gateway.ex +++ b/elixir/apps/domain/lib/domain/cache/gateway.ex @@ -66,19 +66,15 @@ defmodule Domain.Cache.Gateway do now_unix = DateTime.utc_now() |> DateTime.to_unix(:second) # 1. Remove individual flows older than 14 days, then remove access entry if no flows left - cache - |> Enum.map(fn {tuple, flow_id_map} -> - flow_id_map = - Enum.reject(flow_id_map, fn {_fid_bytes, expires_at_unix} -> - expires_at_unix < now_unix - end) - |> Enum.into(%{}) - - {tuple, flow_id_map} - end) - |> Enum.into(%{}) - |> Enum.reject(fn {_tuple, flow_id_map} -> map_size(flow_id_map) == 0 end) - |> Enum.into(%{}) + for {tuple, flow_id_map} <- cache, + filtered = + Map.reject(flow_id_map, fn {_fid_bytes, expires_at_unix} -> + expires_at_unix < now_unix + end), + map_size(filtered) > 0, + into: %{} do + {tuple, filtered} + end end @doc """