Bug #17114

For large shared projects wb2 is loading very slow

Added by Daniel Kutyła 11 months ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
11/24/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

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

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

Task #17115: Review 17114-side-panel-tree-as-listResolvedDaniel Kutyła

Associated revisions

Revision eb7235a7
Added by Daniel Kutyła 11 months ago

Merge branch '17114-side-panel-tree-as-list'
Refs #17114

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

History

#1 Updated by Peter Amstutz 11 months 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

#2 Updated by Lucas Di Pentima 11 months 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.

#3 Updated by Peter Amstutz 11 months 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.

#4 Updated by Peter Amstutz 11 months ago

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

#6 Updated by Lucas Di Pentima 11 months 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.

#7 Updated by Lucas Di Pentima 11 months 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?

#8 Updated by Lucas Di Pentima 11 months 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.

#9 Updated by Lucas Di Pentima 11 months 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.

#11 Updated by Lucas Di Pentima 11 months 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.

#12 Updated by Peter Amstutz 11 months ago

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

#13 Updated by Peter Amstutz 10 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF