Project

General

Profile

Actions

Bug #17114

closed

For large shared projects wb2 is loading very slow

Added by Daniel Kutyła about 4 years ago. Updated 7 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Story points:
-
Release relationship:
Auto

Description

App tends to create a lot of html nodes and removes them after some time this makes it unresponsive and slow


Files

filter-tree-original.png (18.6 KB) filter-tree-original.png Lucas Di Pentima, 11/24/2020 07:02 PM
filter-tree-broken.png (18.9 KB) filter-tree-broken.png Lucas Di Pentima, 11/24/2020 07:02 PM
Screen Shot 2020-11-26 at 12.51.51.png (218 KB) Screen Shot 2020-11-26 at 12.51.51.png Metrics results screenshot Lucas Di Pentima, 11/26/2020 03:53 PM
review-17114.ods (14.7 KB) review-17114.ods Metrics original spreadsheet Lucas Di Pentima, 11/26/2020 03:53 PM
changing-projects-profile-screenshot.png (1.18 MB) changing-projects-profile-screenshot.png Lucas Di Pentima, 11/27/2020 01:03 PM
switching-projects-2-times-profile.json.zip (10.2 MB) switching-projects-2-times-profile.json.zip Lucas Di Pentima, 11/27/2020 01:04 PM

Subtasks 1 (0 open1 closed)

Task #17115: Review 17114-side-panel-tree-as-listResolvedDaniel Kutyła11/24/2020Actions
Actions #1

Updated by Peter Amstutz about 4 years ago

From discussion:

  • Showing 100s or 1000s of nodes into the tree isn't good UI, could limit the number of nodes to make tree more efficient
  • Seems like it builds the whole tree (?) and then collapses nodes, lots of overhead for nodes never visible
  • Tree is built using a very inefficient method that makes many copies and creates lots of garbage
  • Could migrate to virtual tree component that we use for file listing
Actions #2

Updated by Lucas Di Pentima about 4 years ago

If we're going to allow showing projects in the order of 1000 or more, a virtual list will be needed nevertheless, because AFAIR on the file listing case, it used to get really slow with not so many files.

Here're some metrics: https://dev.arvados.org/issues/15610#note-26

OTOH I like Peter's suggestion on not rendering under "shared with me" (for example) more than 50 elements or something like that, but I think the UI has to be very clear as to why it isn't rendering the rest, or maybe if there're more than N, take the user to the main "shared with me" panel UI and not render that branch of the tree.

Actions #3

Updated by Peter Amstutz about 4 years ago

FWIW, React Material UI has a Tree View in the "Lab" section, which means it isn't a core component. That's probably why it the original WB2 team built their own.

https://material-ui.com/components/tree-view/

We could look into this one, but the virtualized tree component we adopted for the file browser might still be even better.

Actions #4

Updated by Peter Amstutz about 4 years ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint
Actions #6

Updated by Lucas Di Pentima almost 4 years ago

Reviewing arvados-workbench2|8fd2c92
Branch 17114-side-panel-tree-as-list

  • The Tree component is used on several parts, including the “type filter selector” on DataExplorer UIs. As you can see on the attached screenshots, that selector is now broken because the user cannot select which kind of process or data collection to show.
Actions #7

Updated by Lucas Di Pentima almost 4 years ago

While I do the code review: Can you provide metrics on how many times this fix is faster than the previous implementation, and if possible, how it's related to the size of the rendered subtree?

Actions #8

Updated by Lucas Di Pentima almost 4 years ago

I think it's really important to take measurements and build metrics so we can compare the before/after situation in an objetive manner and not by feeling.

I've taken the time to make these measurements. Here's the testing scenario:

  • Cluster: ce8i5.arvadosapi.com
  • Test project hierarchies (10000 projects each - you should be able to access them as I've shared them for read access):
  • Measurement: "Time to interactive" (TTI) as the time to load the page and it's consistently ready for user interaction. For this I used Chrome's DevTools Performance tab.
  • Method: both master and branch versions were tested against the remote ce8i5 cluster using Chrome browser.

I'm attaching the result's screenshot and the original spreadsheet, but what I've seen is that the improvement although considerable, seems to be not enough:

9% time reduction in the deep tree case, and 30% in the shallow case.

Even with the 30% TTI reduction, the user has to wait for almost 15 seconds for the UI to stabilize, and that I think isn't a good enough user experience.

Actions #9

Updated by Lucas Di Pentima almost 4 years ago

Some code-related comments:

  • There’s commented out code in file src/views-components/tree-picker/tree-picker.ts line 24
  • I think that renderSizePanelItem() doesn’t need to be exported at file src/views-components/side-panel-tree/side-panel-tree.tsx line 44
  • Could we use symbols instead of strings for TOGGLE_OPEN and TOGGLE_ACTIVE at file src/components/tree/tree.tsx? I think that helps keeping the code more readable.
Actions #11

Updated by Lucas Di Pentima almost 4 years ago

To recap yesterday's video chat about the topic:

  • Renders got way down with the current branch fix: from 20k to 1k or something along those lines. (really good improvement!)
  • The user currently suffering this issue is also probably still using an old arvados version with a bug that made the API server include the manifest_text field when returning collections as part of group/project contents. This clogs the HEAP and makes the browser go too much GC'ing, blocking the UI in the process.
  • Daniel's development machine is resource constrained and that exacerbates the differences between the "before and after" code, taking a totally unusable version to a seemingly super fast (in comparison) experience. (at least IMO)
Today I did some more UX testing:
  • After interacting with the UI, it's confirmed that the total used heap goes no more than ~90MB, this is really good.
  • Once the first render happens (for example, by hitting the reload button), changing from project to project by clicking on the left side panel is "less slow" IMO, not yet snappy enough.
  • I have a local arvbox instance with an hierarchy of 100 projects with 50 subprojects each and changing between projects takes me around 8-10 from click to total UI responsiveness (see attached Chrome DevTools Performance profile image, entire profile file also provided)

So, I would say that the current branch is ready to merge, but I'm not sure that the UI slowness when having too many projects is yet solved. I would advise to keep this ticket open while we wait for user feedback.

Actions #12

Updated by Peter Amstutz almost 4 years ago

  • Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint
Actions #13

Updated by Peter Amstutz almost 4 years ago

  • Status changed from In Progress to Resolved
Actions #14

Updated by Peter Amstutz 7 months ago

  • Release set to 37
Actions

Also available in: Atom PDF