Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid O(n^2) time broadcast' by replacing Writer #112

Merged
merged 3 commits into from
May 9, 2023

Conversation

robbert-vdh
Copy link
Member

@robbert-vdh robbert-vdh commented May 9, 2023

As mentioned by @fatho in #102, the Writer monad can be replaced with the strict state monad to avoid the use for mappend/list concatenation for every element added to the list.. This brings the time complexity down from O(n^2) to O(n). I added the reverse so the order stays the same before and after this change. If that's not needed the reverse can also be removed. Then order of the lists in the tests would then need to be changed.

I thought about removing the monad entirely since it's only used for keeping track of the list and the return value is never used, but an alternative version that still runs on O(n) time would require passing the list around directly which gets very messy.

Resolves #102.

@robbert-vdh robbert-vdh requested a review from diegodiv May 9, 2023 11:05
@diegodiv
Copy link
Contributor

diegodiv commented May 9, 2023

The code looks ok to me but how did you test it?

@robbert-vdh robbert-vdh force-pushed the robbert-vdh/fix/broadcast-complexity branch from c204311 to aa3320b Compare May 9, 2023 11:20
This prevents us from making sure the order doesn't change when
`broadcast'` is modified.
@robbert-vdh
Copy link
Member Author

I thought the SubscriptionTree tests already tested the broadcast' function directly. I was going to add a regression test to make sure the order doesn't change but if it did this test would fail:

it "notifies parents and children about updates" $ do
broadcast'' ["foo", "bar"]
`shouldBe` [ (conn1, value)
, (conn2, value_foo)
, (conn3, value_foo_bar)
]

But, there's a sneaky sort right here:

broadcast'' path = sortOn fst $ broadcast' path value root

I added another commit before this one that removes the sort so the order is also tested (if you change the orders in broadcast' it will break the test). The CI won't run for that commit but the tests are all green for both commits!

With the strict state monad we can prepend the items to a list and then
reverse the list afterwards to maintain the original order. This brings
the time complexity down from `O(n^2)` (because of the mappend/list
concatenation used by Writer) to `O(n)`.

Resolves #102.
@robbert-vdh robbert-vdh force-pushed the robbert-vdh/fix/broadcast-complexity branch from bd83c69 to c525dcc Compare May 9, 2023 13:05
Copy link
Contributor

@diegodiv diegodiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@@ -89,7 +88,7 @@ spec = do
value_foo_bar = AE.Null
value_baz = AE.object []

broadcast'' path = sortOn fst $ broadcast' path value root
broadcast'' path = broadcast' path value root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@robbert-vdh
Copy link
Member Author

@OpsBotPrime merge

@OpsBotPrime
Copy link
Contributor

Pull request approved for merge by @robbert-vdh, rebasing now.

Approved-by: robbert-vdh
Auto-deploy: false
@OpsBotPrime
Copy link
Contributor

Rebased as a24b517, waiting for CI …

@OpsBotPrime
Copy link
Contributor

CI job 🟡 started.

@OpsBotPrime OpsBotPrime merged commit a24b517 into master May 9, 2023
@OpsBotPrime OpsBotPrime deleted the robbert-vdh/fix/broadcast-complexity branch May 9, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Writer to the CPS version in mtl-2.3.1
3 participants