fix(alerts): performance improvements for filtering alerts by informed entity values by rudiejd · Pull Request #1100 · mbta/api

rudiejd · 2026-10-01T20:16:41Z

Summary of changes

Asana Tecket: 🍎 Figure out what went wrong with removing alerts hook

Problem:
Long, meandering Slack thread in which I crash out here: https://mbta.slack.com/archives/C06D663HRRN/p1790722806365799

The main performance problem boils down to the fact that nil gets repeated in the State.Alert.InformedEntity matchers for each element in a list.
For example, if you pass filter[stop_id]=1,2,3,4&filter[trip_id]=1,2,3,4 you end up with matchers for the collections [1, nil, 2, nil, 3, nil, 4, nil] * (cartesian product) 1,[nil, 2, nil, 3, nil, 4, nil]. So you get 8 * 8 = 64 matchers instead of what you want, which is [1, 2, 3, 4, nil] * [1, 2, 3, 4, nil] or 25 matchers.

Solution:
Make the matchers unique. This is only a linear improvement in runtime with respect to number of items in the list filters. I expect that you could get an exponential improvement (e.g. O(n^2) matchers -> O(1) matchers) if you used :ets.fun2ms or Ex2ms to generate one matcher that satisfies all conditions.

Also, I set the key for the InformedEntity ETS table to be the alert ID, which I think will result in more efficient in-memory representation of the table. The old key was the name of the struct (ETS defaults to using the first item in the tuple as the key), which obviously is not unique.

For future work, I expect that you could get the growth of matchers with respect to input list items from O(n^2) to O(1) if you used :ets.fun2ms/1 or Ex2ms to generated a compound match expression that covers all of the criteria instead of trying to create many matchers.

🤖 AI Disclosure: I was banging my head against the wall on this and used copilot to point me at the right line. I already knew basically what file the problem was in, and what category of problem it was. So I pointed Copilot at informed_entity.ex and asked for ideas on what could degrade ETS performance. Then I profiled some of the suggestions. The code written is all my own.

--

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

rudiejd · 2026-10-01T20:57:58Z

  end

  describe "match/1" do
    test "can return facility alerts" do


the order of matchers shouldn't matter anyway here since they are ALL being applied (in other words, all matchers are logically OR'd together like trip_id == 1234 AND stop_id == nil OR trip_id == nil AND stop_id == 1234... ad. inf.) and the set isn't ordered

jadrian · 2026-10-02T14:57:46Z

-        facility: facility
-      }
+        facility <- part_values(matcher, :facility),
+        reduce: MapSet.new() do


Instead of reduce, I think you can use uniq: true. https://elixir.hexdocs.pm/Kernel.SpecialForms.html#for/1-the-into-and-uniq-options

Though see my next comment; I think you want to unique-ify things at a higher call level.

jadrian · 2026-10-02T15:03:38Z

-      }
+        facility <- part_values(matcher, :facility),
+        reduce: MapSet.new() do
+      # we create one nil selector for each parameter that's passed in


I don't see how the duplication you've described is happening here. IIUC, part_values() returns at worst a two-element list with nil as the second element. Since there are no duplicated nils in any one list, then there is no duplication in the cartesian product of the generators, and so no reason to unique-ify here.

good catch on this! I originally was doing Enum.uniq\1 in the match function, but I decided to get cute and evidently ended up doing it in the wrong place. 5fb6ab0

This change the changes in the tests redundant

rudiejd · 2026-10-02T17:29:26Z

Load test results.

Query: https://api-dev-green.mbtace.com/alerts?filter%5Bstop%5D=70079,70080,74611,74617,84611,NEC-2287,NEC-2287-01,NEC-2287-02,NEC-2287-03,NEC-2287-04,NEC-2287-05,NEC-2287-06,NEC-2287-07,NEC-2287-08,NEC-2287-09,NEC-2287-10,NEC-2287-11,NEC-2287-12,NEC-2287-13,32001,32002,32003,32004,32005,70101,70102,MM-0079-S&filter%5Btrip%5D=76734708,76734709,SouthBase-826039-1925,ADDED-1584585658,76734711,ADDED-1584585797,SouthBase-825905-1027,76734713,76734714,SouthBase-825930-1077

master/prod:


  █ TOTAL RESULTS

    HTTP
    http_req_duration..............: avg=4.51s min=1.42s med=2.95s max=17.29s p(90)=10.38s p(95)=11.33s
      { expected_response:true }...: avg=4.51s min=1.42s med=2.95s max=17.29s p(90)=10.38s p(95)=11.33s
    http_req_failed................: 0.00%  0 out of 201
    http_reqs......................: 201    4.019886/s

    EXECUTION
    iteration_duration.............: avg=5.02s min=1.98s med=3.45s max=17.79s p(90)=10.93s p(95)=11.89s
    iterations.....................: 197    3.939889/s
    vus............................: 1      min=1        max=49
    vus_max........................: 50     min=50       max=50

    NETWORK
    data_received..................: 3.2 MB 63 kB/s
    data_sent......................: 143 kB 2.9 kB/s

feature/dev-green:

  █ TOTAL RESULTS

    HTTP
    http_req_duration..............: avg=2.45s min=322.18ms med=2.21s max=7.82s p(90)=4.57s p(95)=5.01s
      { expected_response:true }...: avg=2.45s min=322.18ms med=2.21s max=7.82s p(90)=4.57s p(95)=5.01s
    http_req_failed................: 0.00%  0 out of 364
    http_reqs......................: 364    7.279849/s

    EXECUTION
    iteration_duration.............: avg=2.98s min=823.44ms med=2.75s max=8.33s p(90)=5.08s p(95)=5.53s
    iterations.....................: 356    7.119853/s
    vus............................: 1      min=1        max=49
    vus_max........................: 50     min=50       max=50

    NETWORK
    data_received..................: 13 MB  257 kB/s
    data_sent......................: 209 kB 4.2 kB/s

jadrian · 2026-10-02T17:41:59Z

+      # we create one nil selector for each parameter that's passed in
+      # so when a list is passed in, there is a nil selector for every item in the list
+      # meaning with a list length of N there are N - 1 redundant selectors)
+      # hence the use of a map set to remove duplicates here


Please update this comment; you're no longer removing duplicates. In fact I think you could revert the whole comment entirely, or perhaps adapt it to the comprehension on line 19.

jadrian · 2026-10-02T17:45:08Z

@@ -32,27 +32,34 @@ defmodule State.Alert.InformedEntityTest do

  describe "match/1" do


Is there any way to create a test that would only pass with your change in place?

I don't think we can easily do this without leaking some of the private details of the InformedEntiy module. I think the more important thing to test (performance) is non-functional here, and is validated by the load tests

jadrian · 2026-10-02T19:21:29Z


  def new(table \\ @table) do
-    ^table = :ets.new(table, [:named_table, :duplicate_bag, {:read_concurrency, true}])
+    # keypos 2 here sets the key for the ETS table to be the alert ID, which makes selecting by alert ID faster


Nit: please wrap at 100 characters

jadrian · 2026-10-02T19:22:55Z

+        # we create one nil selector for each parameter that's passed in
+        # so when a list is passed in, there is a nil selector for every item in the list
+        # meaning with a list length of N there are N - 1 redundant selectors)
+        # hence the use of a map set to remove duplicates here


s/a map set to remove duplicates/uniq/

jadrian · 2026-10-02T19:23:31Z

+          uniq: true do
+        # we create one nil selector for each parameter that's passed in
+        # so when a list is passed in, there is a nil selector for every item in the list
+        # meaning with a list length of N there are N - 1 redundant selectors)


Please fix dangling close-paren

jadrian · 2026-10-02T20:25:43Z

+        # We create one nil selector for each combination in the Cartesian product of
+        # parameters that are passed in, with one `nil` for each element in each list. For example,
+        # with stops 1, 2 and trips 1, 2 we get all of these combinations including nil:
+        # [(1, nil), (1, nil), (2, nil), (2, nil), (nil, 1), (nil, 1), (nil, 2) (nil, 2)].


Nit: I understand this comment but I think if I hadn't been reviewing this PR, it would confuse me. A small change would clarify: "stops 1, 2 and trips A, B".

Also, wouldn't we get an additional four copies of a (nil, nil) combination?

rudiejd requested a review from a team as a code owner October 1, 2026 20:16

rudiejd added this pull request to stack #1101 October 1, 2026 20:16

rudiejd mentioned this pull request Oct 1, 2026

fix(alerts): remove the alerts hooks again #1098

Open

Base automatically changed from chore/update-mint to master October 1, 2026 20:33

rudiejd force-pushed the fix/improve-alerts-filter-performance branch 2 times, most recently from 44a3512 to f5791bb Compare October 1, 2026 20:33

rudiejd commented Oct 1, 2026

View reviewed changes

rudiejd requested review from a team and jadrian and removed request for a team October 1, 2026 23:08

jadrian requested changes Oct 2, 2026

View reviewed changes

rudiejd force-pushed the fix/improve-alerts-filter-performance branch 2 times, most recently from e269be6 to d65f2be Compare October 2, 2026 16:05

rudiejd requested a review from jadrian October 2, 2026 17:40

jadrian requested changes Oct 2, 2026

View reviewed changes

rudiejd requested a review from jadrian October 2, 2026 18:59

jadrian requested changes Oct 2, 2026

View reviewed changes

rudiejd force-pushed the fix/improve-alerts-filter-performance branch from 34754fb to 53c7907 Compare October 2, 2026 20:03

rudiejd requested a review from jadrian October 2, 2026 20:12

rudiejd added 5 commits October 2, 2026 16:23

fixup: performance improvements!

4fbd4bd

fixup: apply unique on larger collection of matchers

d61ccdb

fixup: fix tests again

f7a7196

fixup: move comment to relevant section

9e5305f

fixup: revise comment

6b92efa

rudiejd force-pushed the fix/improve-alerts-filter-performance branch from 53c7907 to 6b92efa Compare October 2, 2026 20:23

jadrian approved these changes Oct 2, 2026

View reviewed changes

rudiejd added 2 commits October 2, 2026 16:47

fixup: make comment less confusing

72655a2

fixup: make matcher more accurate

0422d4d

jadrian approved these changes Oct 2, 2026

View reviewed changes

		@@ -32,27 +32,34 @@ defmodule State.Alert.InformedEntityTest do

		describe "match/1" do

Conversation

rudiejd commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

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!

rudiejd commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rudiejd commented Oct 1, 2026 •

edited

Loading

rudiejd commented Oct 2, 2026 •

edited

Loading