Bug #17114
closedFor large shared projects wb2 is loading very slow
Description
App tends to create a lot of html nodes and removes them after some time this makes it unresponsive and slow
Files
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
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.
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.
Updated by Peter Amstutz about 4 years ago
- Target version changed from 2020-11-18 to 2020-12-02 Sprint
Updated by Daniel Kutyła almost 4 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/8fd2c927929859ec4905d7c50c83d9dd170cc6d9
Test run: developer-tests-workbench2: #187
More cleanup
Updated by Lucas Di Pentima almost 4 years ago
- File filter-tree-broken.png filter-tree-broken.png added
- File filter-tree-original.png filter-tree-original.png added
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.
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?
Updated by Lucas Di Pentima almost 4 years ago
- File review-17114.ods review-17114.ods added
- File Screen Shot 2020-11-26 at 12.51.51.png Screen Shot 2020-11-26 at 12.51.51.png added
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):
- Shallow hierarchy: ce8i5-j7d0g-e6a4l7kn7eu8um1 (100 projects with 100 subprojects each)
- Deep hierarchy: ce8i5-j7d0g-93r09l3eydh39fp (10 projects with 10 subprojs with 10 subprojs with 10 subprojs each)
- 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.
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 filesrc/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.
Updated by Daniel Kutyła almost 4 years ago
New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/86d0ec68c0a400ab6d4de1897a68a4593f450b60
Test run: developer-tests-workbench2: #197
Post review cleanup
Updated by Lucas Di Pentima almost 4 years ago
- File switching-projects-2-times-profile.json.zip switching-projects-2-times-profile.json.zip added
- File changing-projects-profile-screenshot.png changing-projects-profile-screenshot.png added
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)
- 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.
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint
Updated by Peter Amstutz almost 4 years ago
- Status changed from In Progress to Resolved