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

High energy impact when Talk tab is visible, none when invisible #4501

Closed
PVince81 opened this issue Oct 30, 2020 · 10 comments
Closed

High energy impact when Talk tab is visible, none when invisible #4501

PVince81 opened this issue Oct 30, 2020 · 10 comments
Assignees
Labels
Milestone

Comments

@PVince81
Copy link
Member

Steps to reproduce

  1. Open Talk app in a first Firefox window
  2. Create a few rooms with multiple people
  3. Refresh the page
  4. Open a second Firefox window and open "about:performance" in it, highlight the entry for the Talk app and observe: sustained medium impact
  5. In the first window, create a new empty tab and switch to it, this makes the Talk tab invisible: low impact
  6. Switch back to the Talk tab: sustained medium impact again

Note: if the medium impact doesn't happen, try switching between multiple rooms, then observe again.

Expected behaviour

Low performance impact when no poll request is happening.

Actual behaviour

Medium performance impact despite no network requests happening.

Talk app

Talk app version: 10.0.0 (on NC's company cloud) and 11.0 pre-alpha on git (a9fb5ba)

Custom Signaling server configured: yes for NC cloud, no for local one.

Notes

This is important nowadays because people might leave a Talk window present on another monitor like I do, or another virtual desktop (on Linux)

I tried the performance graph tool from Firefox but could not yet found anything significant apart from some GC triggers.

@PVince81 PVince81 self-assigned this Oct 30, 2020
@PVince81 PVince81 added this to the 💚 Next Major (21) milestone Oct 30, 2020
@PVince81
Copy link
Member Author

Doesn't happen when loading Talk without entering a room.
It's once a room has been entered that it happens.

I had one rare case where I first joined an existing 1-1 room and the impact was still low. Then after switching between rooms and coming back it went up to medium again.

I've tried hiding all visible elements with "display:none" but the impact is still there: so it's not a rendering issue.

@PVince81
Copy link
Member Author

Perf debugger when tab is active:
image

Sadly most is empty space.

And here when the tab is unfocussed:
image

If I comment out the "ChatView" in "MainView" so it doesn't get rendered, the problem disappears.
So it's not only about hiding visually but also the absence of matching DOM elements.

@PVince81
Copy link
Member Author

removing NewMessageForm solves the problem... getting closer...

@PVince81
Copy link
Member Author

EmojiPicker ❓ ❗ 💥 🤯 🔥

@PVince81
Copy link
Member Author

Deleting the DOM nodes of the EmojiPicker in the inspector did not solve the issue.
So it must be in the popup itself.

I've checked the EmojiPicker in the Vue style guide here https://nextcloud-vue-components.netlify.app/#/Components/EmojiPicker and despite its presence the performance impact is not observed there.

So it must be something specific to the layout here in Talk

@PVince81
Copy link
Member Author

I went up to EmojiPicker.vue and the problem disappeared if I removed the Picker element.
Removing only the CSS (including the import from the lib) in EmojiPicker.vue didn't help.

The Picker belongs to the library https://github.com/serebrov/emoji-mart-vue

@PVince81
Copy link
Member Author

Updating to the latest version (7.0.7) did not help.

If I replace "Picker" with "StaticPicker" in nextcloud-vue, the performance problem disappears.

I saw that "Picker" is using "DynamicScroller" aka virtual scrolling, and maybe somehow that bit is doing something in the background... or causing those repeated GC events, even though nothing is rendered yet.

Visually, the static picker looks the same and also has a search box.
If we don't find a better solution I suggest we simply make that switch.

Also see serebrov/emoji-mart-vue#80

@skjnldsv @ma12-co

@PVince81
Copy link
Member Author

PR here: nextcloud-libraries/nextcloud-vue#1526

@jospoortvliet
Copy link
Member

great research @PVince81 !

@PVince81
Copy link
Member Author

PVince81 commented Nov 2, 2020

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

No branches or pull requests

2 participants