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.

Sign up for GitHub

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

Jump to bottom

parallelize broadcast room join #5717

Merged
merged 1 commit into from
Mar 19, 2020
Merged

parallelize broadcast room join #5717

merged 1 commit into from
Mar 19, 2020

Conversation

hackaugusto
Copy link
Contributor

Joining rooms is a bit slow, this parallelizes it, should make the first run a tidy bit faster (roughly 10 seconds).

This also moves the sync code to a separate init function with explicit assumption checks.

@hackaugusto hackaugusto requested a review from ulope January 21, 2020 10:53
@auto-assign auto-assign bot requested a review from rakanalh January 21, 2020 10:54
ulope
ulope approved these changes Jan 21, 2020
View reviewed changes
Copy link
Collaborator

@ulope ulope left a comment

Choose a reason for hiding this comment

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

LGTM.

One suggestion and two typos.

@hackaugusto hackaugusto force-pushed the parallel_broadcast_room_join branch from 64ecd03 to 84ad72d Compare January 21, 2020 19:01
@codecov
Copy link

codecov bot commented Jan 21, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@445f3a6). Click here to learn what that means.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #5717   +/-   ##
==========================================
  Coverage           ?   78.64%           
==========================================
  Files              ?      133           
  Lines              ?    15315           
  Branches           ?     2338           
==========================================
  Hits               ?    12045           
  Misses             ?     2585           
  Partials           ?      685
Flag Coverage Δ
#integration 72.03% <96.42%> (?)
#unit 55.55% <3.57%> (?)
Impacted Files Coverage Δ
raiden/network/transport/matrix/transport.py 85.75% <96.42%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 445f3a6...6f91485. Read the comment docs.

@hackaugusto hackaugusto force-pushed the parallel_broadcast_room_join branch from 84ad72d to e944443 Compare January 28, 2020 10:57
@hackaugusto hackaugusto force-pushed the parallel_broadcast_room_join branch 2 times, most recently from 78275d7 to 6fa53f1 Compare February 11, 2020 13:34
@fredo
Copy link
Contributor

fredo commented Feb 20, 2020

we should think about if this is still necessary once #5892 is merged

@Dominik1999 Dominik1999 removed the request for review from rakanalh February 24, 2020 15:55
@Dominik1999
Copy link
Contributor

What needs to be done here? Fix the test or answering Fred's comment?

@fredo
Copy link
Contributor

fredo commented Feb 27, 2020

This morning I analyzed if it is useful to still parallelize _initialize_broadcast_rooms()

Some facts that should help us decide:

  • Joining a broadcast room cost 2 API calls (join and get_alias)
  • After the latest changes joining the broadcast rooms is done before the first_sync
  • Note that, joining a room takes a little bit longer if the user has not joined yet

I did a little run to compare the blocking joins and parallel joins. The results are the following:

-------------BLOCKING------------
AVERAGE: 2.9611104428499404
MEAN: 2.5610763099994074
-----------ASYNC----------
AVERAGE: 1.2217540442000427
MEAN: 0.9199940479993529

IMO The results make sense since one join contains of two API calls which took me roughly 0.3 to 0.5 seconds each. This counts up to ~0.8 seconds for each join. These numbers are reflected in the ASYNC time result since all 3 rooms are joined in parallel. The blocking time result is roughly a factor 3 which is due to the fact that we have 3 broadcast rooms to join.

Since the whole complexity of joining broadcast rooms is separated from the sync processes since #5931 and we can establish the broadcast room join before even calling the first sync the time consumption is not that big of a deal IMO. But saving 1.5 s at startup is still a nice goodie. Because of that, I would opt for implementing this feature since it already has a PR.

@fredo fredo force-pushed the parallel_broadcast_room_join branch from 6fa53f1 to b7d467a Compare February 27, 2020 11:19
@fredo fredo force-pushed the parallel_broadcast_room_join branch 2 times, most recently from 39513ac to 45351ce Compare March 19, 2020 11:12
@fredo
parallelize broadcast room join
6f91485 
Join all broadcast rooms in parallel
@fredo fredo force-pushed the parallel_broadcast_room_join branch from 45351ce to 6f91485 Compare March 19, 2020 11:59
hackaugusto
hackaugusto commented Mar 19, 2020
View reviewed changes
raiden/network/transport/matrix/client.py
@@ -104,21 +104,22 @@ def update_local_aliases(self) -> bool:
"""
server_name = urlparse(self.client.api.base_url).netloc
changed = False
for event_type in ["m.room.aliases", "m.room.canonical_alias"]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why don't we need the canonical aliases anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use them anymore

raiden/network/transport/matrix/client.py
if "alias" in response:
if self.canonical_alias != response["alias"]:
self.canonical_alias = response["alias"]
changed = True
if changed and self.aliases and not self.canonical_alias:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible for a room to not have a canonical alias?

raiden/network/transport/matrix/transport.py
"sync thread, since that is necessary to properly generate the "
"filters."
)
assert self._broadcast_rooms, msg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this right? do we join the broadcast rooms if the node is not configured to use the ms and pfs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will always join at least the discovery room.

@hackaugusto hackaugusto merged commit 661da94 into raiden-network:develop Mar 19, 2020
@hackaugusto hackaugusto deleted the parallel_broadcast_room_join branch March 19, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulope ulope ulope approved these changes

@fredo fredo fredo left review comments

Assignees

@fredo fredo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hackaugusto @fredo @Dominik1999 @ulope