Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Riot should ignore events it cannot process (IoT) #486

Closed
turt2live opened this issue Feb 18, 2017 · 12 comments
Closed

Riot should ignore events it cannot process (IoT) #486

turt2live opened this issue Feb 18, 2017 · 12 comments
Labels
A-Performance A-Timeline O-Occasional Affects or can be seen by some users regularly or most users rarely S-Tolerable Low/no impact on users T-Defect Z-MadLittleMods

Comments

@turt2live
Copy link
Member

turt2live commented Feb 18, 2017

bounty

Description

TL;DR: I made something that sent 43,200 custom events and Riot doesn't like me anymore.

Backstory: I started work on matrix-temperature-logger - a project to record basic weather data to a Matrix room for processing by other parties. The project uses a custom event (io.t2l.matrix.weather) to send this data to the room. Riot apparently tries to cache these events, despite it not knowing how to handle them, leading to some pretty significant degradation of performance as time goes on.

The project currently sends 1 event per second, and after 12 hours of run time that's ~43,200 events that have ended up in the room. The new indexeddb improvements help a lot with processing the thousands of events, however it still seems to reduce performance whenever I enter that room.

Each time I click on that room, I expect to see the few state events and couple messages that Riot can show me, however the spinner goes wild (flashing in and out of view) while it tries to load the thousands of weather events. As it loads more and more, the spinner gets slower and slower (and the UI gets less and less responsive). At the time of writing this, it takes about 5 minutes for Riot to show me everything in the room (see screenshot).

Note: I fully recognize that 43,200 invisible events is ridiculous and may even be abusing the intended use case of Matrix. However, I feel as though Riot should intentionally drop these messages from it's cache and pretend they never happened so the room is still usable as a communication room (which is what I was going for - people chatting with weather data silently being transferred without the participants noticing, unless you're the bot handling it all).

When trying to investigate what was causing the spinner to slow down, I found that Riot was generating DOM elements for each event like so: <li data-scroll-token="$14874408815468LsTzb:dev.t2bot.io"></li> - this is probably what caused the increasingly poor performance of the entire application (it also makes Chrome's dev tools very unhappy).

Steps to reproduce

  • Have something that sends a plethora of custom events to a room
  • Let that something run wild to generate tens of thousands of events
  • Try and navigate to the room in Riot
  • Note how the spinner behaves and performance degrades increasingly until it finishes

I do have a test room with these events available. Feel free to ask me (@travis:t2l.io) for an invite if you're looking to avoid the hassle of generating thousands of events.

Describe how what happens differs from what you expected.
I would have expected Riot to ignore the events that it can't handle, but it instead tried to create a timeline out of them.

Log: not sent (can't).

This is what the room looks like after it's done loading the tens of thousands of events:
image

Here's a snippet of the DOM inspector:
image

Version information

  • Platform: web (in-browser)

For the web app:

  • Browser: Chrome 56
  • OS: Windows
  • URL: riot.im/develop
@ara4n
Copy link
Member

ara4n commented Feb 18, 2017

:D so, this isn't abusing Matrix at all - but we obviously haven't optimised for it yet.

@rxl881 this is relevant to your interests ;P

@lukebarnard1 i'm surprised that we're creating an actual EventTile for each of these invisible events - is this deliberate?

I think the correct solution however is to set up Riot to only sync event types it's comfortable with by default. (Although even this might pose problems for Synapse, which probably doesn't perform the filtering very intelligently).

@turt2live
Copy link
Member Author

It appears as though Riot also thinks I have unread messages in the room (when "nothing" has happened). Although that might be related to the event tiles being created (unless I'm totally wrong on how this unread messages notification works).

@ara4n
Copy link
Member

ara4n commented Feb 18, 2017

Thinking some more: we have a proposal for any event to have human-readable representation (including, for instance, your temperature events). This means that we'll need to extend the filter API to say "please send me all visible events".

It could also give you an alternative solution, which is to include human-representable forms of all the events, and then Riot would paginate them sanely and nothing would grind to a halt O:-)

@turt2live
Copy link
Member Author

I would imagine adding a text component to each event would solve the "my browser is unhappy" problem, although it does mean that (at least in my case) the room would be essentially unusable as the bot would be sending "It is currently 24C in the room" constantly (drowning out legitimate messages).

The point of me sending a bunch of data constantly is to have some receiver in the room to process the data. For instance, this could be used to monitor the temperature of a server room - a bot could start pinging people if a temperature threshold is exceeded. Another case (which is actually what I'm working on) is to simply have a record of data to be able to look back on when requested (temperature over time).

Although I suppose this could be combated by having the processing bot in 2 rooms - a data stream room and the "notification" room.

@turt2live
Copy link
Member Author

This seems to also affect riot-android (and probably riot-ios). I can't actually confirm it at the moment, but the room has a spinner at the top that seemingly runs forever.

@ara4n
Copy link
Member

ara4n commented Feb 18, 2017 via email

@rxl881
Copy link

rxl881 commented Feb 19, 2017

Thanks for the heads-up @ara4n, this is definitely relevant to my interests :)

That said, the issues that I was encountering last week were more to do with submission of messages from the client (IoT device) to Synapse, via the JS SDK (submission of the messages doesn't appear to have been a problem here).

@turt2live - I actually quite like the idea of "hidden" messages in the channel being monitored by the bot, but as Ara says, in the short term at least it's probably going to be best to have this high volume traffic in it's own channel. I'll have a chat with Ara and the Matrix team about this in more detail and will let you know if anything changes as we develop our plans and guidance for IoT communication on Matrix.

@lukebarnard1
Copy link

lukebarnard1 commented Feb 21, 2017

// if we aren't showing the event, put in a dummy scroll token anyway, so
// we can scroll to the right place.
ret.push(<li key={eventId} data-scroll-token={eventId}/>);

The above seems a bit pointless because you can't scroll to an invisible event.

We should still alter the filter used on sync because pagination appears to take longer when paginating through invisible events.

TODO:

@lukebarnard1 lukebarnard1 self-assigned this Feb 21, 2017
@lukebarnard1
Copy link

@turt2live has matrix-org/matrix-react-sdk#718 helped at all?

@turt2live
Copy link
Member Author

@lukebarnard1 better, although it still has some performance issues.

For instance, the same element keeps getting re-created so it slows down a little bit.
image
(highlighted elements indicate updates)

The spinner also still flickers, and takes forever, but that seems to be a different issue from what matrix-org/matrix-react-sdk#718 intends to solve.

@lukebarnard1
Copy link

@turt2live It taking forever to paginate is expected because those events will still be paginated across. The re-creating is a bit odd. Perhaps MessageList shouldComponentUpdate needs modification.

@lukebarnard1 lukebarnard1 removed their assignment Nov 6, 2017
@turt2live
Copy link
Member Author

The majority of this has been fixed, although it's still sluggish in rooms with mostly iot activity. Some kind of filtering before the events land in react would probably speed up this process so it's not trying to filter thousands of events in the dom.

Also I've advertised the bounty on the OP.

@element-hq element-hq locked and limited conversation to collaborators Jun 27, 2022
@t3chguy t3chguy converted this issue into discussion #487 Jun 27, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
A-Performance A-Timeline O-Occasional Affects or can be seen by some users regularly or most users rarely S-Tolerable Low/no impact on users T-Defect Z-MadLittleMods
Projects
None yet
Development

No branches or pull requests

6 participants