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

Performance regression with indexeddb during normal operation #3252

Closed
ara4n opened this issue Feb 17, 2017 · 10 comments
Closed

Performance regression with indexeddb during normal operation #3252

ara4n opened this issue Feb 17, 2017 · 10 comments

Comments

@ara4n
Copy link
Member

ara4n commented Feb 17, 2017

I'm seeing the UI lock up for 200-400ms every time a new event is received when using the latest /develop with the IndexedDB stuff. This is made particularly obvious if RoomList is expanded a bit, but it's noticeable anyway. Profiling says that onGlobalMessage chews 100% CPU for about 400ms every time new messages are received.

@kegsay
Copy link
Contributor

kegsay commented Feb 18, 2017

every time a new event is received

I don't believe this is caused by the indexeddb stuff. The code is entirely optimised around making updates to the sync structure fast. There is a known 200-400ms pause when the sync JSON is created and persisted to the database, but that will occur no faster than once every 5 minutes. The sync JSON is done in the callback for /sync though, which may be why you think it's when events are received.

@kegsay
Copy link
Contributor

kegsay commented Feb 20, 2017

Conclusions from #riot-dev:

  • This isn't happening on every event. @ara4n couldn't reproduce it happening as described in this bug.
  • Two CPU profiles were taken and I had a look at them. The first CPU profile contained the aforementioned database pause which lasted 300ms total time. The 2nd CPU profile had no pauses due to indexeddb, but did have pauses due to our old friend refreshRoomList. We've known for ages that this is a the main perf bottleneck in riot-web: this is why rate_limited_func is A Thing in RoomList.js. Riot is listening for too many events and the rate limiting func is there to prevent event spam leading to multiple re-render calls. It seems like we should look into this a bit more. In fact, from what I can tell, every single CPU spike in the 2nd CPU profile has refreshRoomList at its heart.
  • The CPU profiles confirm that accumulation of sync data (which as I said was designed to be fast) is not related to this: it took <2ms total time (<0.1ms self) to execute.

@ara4n
Copy link
Member Author

ara4n commented Mar 16, 2017

I think the delays may be related to doing GCs due to leaking/churning RoomMembers everywhere (110K room members in ~20 minutes on my account), which is in turn #3307

@kegsay
Copy link
Contributor

kegsay commented Mar 16, 2017

I think the delays may be related to doing GCs

The CPU profiles you did a while back refute that though:

1096.2ms 4.02 %	(garbage collector)

@lampholder lampholder added this to the Riot web next release milestone Mar 27, 2017
@lampholder
Copy link
Member

This could be related to #3517

@ara4n
Copy link
Member Author

ara4n commented Mar 27, 2017

Various possible remaining problems here:

Suggestion is for Dave to look into the webworker thing.

@ara4n ara4n assigned dbkr and unassigned kegsay Mar 27, 2017
@lampholder
Copy link
Member

We're going to keep observing this, leaving it until the end of this milestone. We'll make a decision based on our experience with it up 'til then.

@lampholder
Copy link
Member

We're going to keep this in the current milestone and try and resolve before the next release.

@dbkr
Copy link
Member

dbkr commented Apr 6, 2017

Indexeddb will now run in a web worker: lets see if this makes this better

@lampholder
Copy link
Member

We have concluded that this does indeed make this better 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants