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

Opening buffers whose rooms have very large numbers of adjacent membership events can be very slow #247

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alphapapa
Copy link
Owner

OS/platform

Debian testing

Emacs version and provenance

29.1, installed as a package from Debian testing.

Emacs command

emacs --fg-daemon

Emacs frame type

Gui frame, pgtk

Ement package version and provenance

0.13, as a package from Debian testing

Actions taken

Pressed enter in the room list buffer to open a room buffer

Observed results

It took slightly over 7 minutes (with Emacs using 100% cpu for the whole time) to open the buffer for a room with 221 members. The room is bridged to IRC, so it sees a lot of joining and leaving (I assume processing these is the problem).

Expected results

Most rooms only take a few seconds to open. Getting below a minute would already be quite useful.

Backtrace

No response

Etc.

Adding a per-room or global option to not process all of the membership changes at buffer open time would be a workaround.

I wonder if this is actually necessary at all, though. If I press C-g while this is happening the buffer opens just fine, it just doesn't contain the list of members at the top.

@viiru- viiru- added the bug Something isn't working label Dec 9, 2023
@alphapapa
Copy link
Owner

This is certainly an unusual case. I've been in a variety of bridged rooms with many more users (e.g. #emacs:libera.chat), and I never saw such a long delay when opening the room.

I suppose this might be caused by the recent disabling of the matrix.org public bridge, causing many adjacent membership events, which Ement coalesces into a single rendered event; the algorithm is naive and re-renders the event as each new event is coalesced into it, and each sub-event is also rendered and applied as a text property to the relevant username, so it's probably something like O(n^2) or worse. Outside of extraordinary circumstances, it's not a problem, but I suppose these are extraordinary circumstances. I could imagine hundreds or thousands of such events taking a long time, especially as each event is formatted into a string and then garbage collected and formatted again...

The best way to solve this would be to render the coalesced events once, after they have all been coalesced, but that would require some refactoring to implement, as we have no support for any kind of deferred rendering. Perhaps an even better solution would be to implement a way to have multiple renderings for a set of events, one being contracted and the other expanded on-demand; again, something we want to have, but a long-term goal.

In the meantime, we could probably implement a failsafe that would avoid rendering sets of coalesced events larger than a certain size; and/or we could limit the number of events that can be coalesced into a single event (which would limit how much re-rendering is done as the events are handled).

I wonder if this is actually necessary at all, though. If I press C-g while this is happening the buffer opens just fine, it just doesn't contain the list of members at the top.

It may also be missing other events that would have been rendered after the membership events.

I am curious to know how many membership events are involved here, though. Would you please evaluate this form in the minibuffer after displaying the room's buffer (aborting the rendering should not affect the result, so no need to wait for that):

(cl-loop for event in (append (ement-room-timeline ement-room)
                              (ement-room-state ement-room))
         count (equal "m.room.member" (ement-event-type event)))

@alphapapa alphapapa changed the title Opening buffers for bridged rooms can take a very long time Opening buffers whose rooms have very large numbers of adjacent membership events can be very slow Dec 9, 2023
@viiru-
Copy link
Contributor Author

viiru- commented Dec 10, 2023

There are two bridged rooms where I always see this issue, one of them (the one I ran this test in) is bridged by someone else between Matrix, Discord and IRC and the other I'm bridging myself from IRC (with heisenbridge).

Running that code in this buffer returns 2112 (#o4100, #x840), which as numbers go doesn't look absurdly large.

@viiru-
Copy link
Contributor Author

viiru- commented Dec 10, 2023

I re-did this with the other room that takes long to open. It currently has 642 members, opening the buffer took almost exactly 15 minutes and the function returned 2533 (#o4745, #x9e5).

@alphapapa
Copy link
Owner

That does seem like an awful lot of membership events. Usually initial sync only includes membership events for users whose normal events have been sent to the client, regardless of how many users are in the room.

Do you know what time span is covered? And, is that the number after initial sync, or after some time has passed? Just curious. (The describe-room buffer shows the timespan of received events.)

alphapapa added a commit that referenced this pull request Dec 11, 2023
@alphapapa
Copy link
Owner

alphapapa commented Dec 11, 2023

@viiru- Please try this branch, which limits each group of coalesced events to 100: https://github.com/alphapapa/ement.el/tree/wip/247-rendering-many-membership-events I feel like that's a reasonable starting value, but it may need to be tuned by experience.

alphapapa added a commit that referenced this pull request Dec 11, 2023
alphapapa added a commit that referenced this pull request Dec 11, 2023
alphapapa added a commit that referenced this pull request Dec 11, 2023
@viiru-
Copy link
Contributor Author

viiru- commented Jan 24, 2024

@viiru- Please try this branch, which limits each group of coalesced events to 100: https://github.com/alphapapa/ement.el/tree/wip/247-rendering-many-membership-events I feel like that's a reasonable starting value, but it may need to be tuned by experience.

Sorry about the ridiculous delay in testing this, things have been rather crazy around here lately.

This branch seems to improve things a great deal, buffers that previously took 15 minutes to open now take about 15 seconds.

@alphapapa
Copy link
Owner

@viiru- No problem, same here.

That is a big improvement. What is the end result like? Does it look reasonable in the buffer?

Also, what if you tune the limit to a smaller value, like 50 or 25? Does that help more? Does it look worse?

In the long run, we could consider trying to implement a deferred rendering of the expansion of coalesced events, so the default appearance would just render a certain string with the number of contained events.

@viiru-
Copy link
Contributor Author

viiru- commented Jan 26, 2024

Perhaps 100 is a bit low, as the buffers opened at that setting don't tend to have much context (actual messages by actual people) visible when I open them. It's not a lot of work to do a couple of pageups to get some, but perhaps I'd be inclined to tune in the other direction.

@phil-s
Copy link

phil-s commented Mar 9, 2024

If the actual JSON parsing is a factor in this issue (?), https://lists.gnu.org/archive/html/emacs-devel/2024-03/msg00244.html may be of interest to some users (only if you're willing to patch and recompile Emacs).

@alphapapa
Copy link
Owner

If the actual JSON parsing is a factor in this issue (?), https://lists.gnu.org/archive/html/emacs-devel/2024-03/msg00244.html may be of interest to some users (only if you're willing to patch and recompile Emacs).

That's not the problem. Even a 20 MB JSON response gets parsed into Lisp objects in about 1 second. The issue is the inefficiency of the event-coalescing code.

I should probably go ahead and merge this, though.

@alphapapa alphapapa added this to the 0.16 milestone Mar 9, 2024
alphapapa added a commit that referenced this pull request May 3, 2024
alphapapa added a commit that referenced this pull request May 3, 2024
@alphapapa
Copy link
Owner

@viiru- Oops, I converted the issue to a PR with Forge (I thought it would just open a PR referencing the issue).

Anyway, I made a small change to ensure that users with the existing option value shouldn't encounter an error. Would you mind testing it once more, please? Then I'll merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:A
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants