-
Notifications
You must be signed in to change notification settings - Fork 27
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
Infinite tree #345
Infinite tree #345
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
BenchmarksSetupBackendThere are multiple ways to prepare the environment for these tests. I will describe how I prepare my environment. While there are probably many better ways of achieving the same result (e.g. $ cd ~/Development/python/workspace/cylc-uiserver
$ git fetch --all ; git checkout master ; git rebase upstream/master
$ virtualenv venv
$ source venv/bin/activate
$ (venv) pip install -e . This should install Cylc UI Server and Cylc Flow. I don't change this, and I call it the "backend". I use the same venv while testing multiple Cylc UI branches. FrontendI don't use $ cd ~/Development/python/workspace
$ cp -r cylc-ui cylc-ui-master # I know, I know... I am sorry. Bear with me.
$ cd cylc-ui-master
$ git fetch --all ; git checkout master ; git rebase upstream/master
$ npm ci
$ npm run build # production mode! Now for the branch that I want to benchmark against. $ cd ~/Development/python/workspace/cylc-ui
$ git fetch --all ; git checkout infinite-tree ; git rebase kinow/infinite-tree
$ npm ci
$ npm run build # production mode! Starting two Cylc UI ServersThis is one of the rare cases where I think you will want to run two Cylc UI Servers. I do that occasionally, and because of that have included instructions how to do that in the Cylc UI Server # `master`
$ cd ~/Development/python/workspace/cylc-uiserver/
$ source venv/bin/activate
$ (venv) cd /tmp
$ (venv) jupyterhub --JupyterHub.spawner_class="jupyterhub.spawner.LocalProcessSpawner" --Spawner.args="['-s', '/home/kinow/Development/python/workspace/cylc-ui-master/dist/']" --Spawner.cmd="cylc-uiserver" --JupyterHub.bind_url="http://:7000" --JupyterHub.hub_bind_url="http://127.0.0.1:7878" --JupyterHub.proxy_api_port="9001" Cylc UI $ cd ~/Development/python/workspace/cylc-uiserver/
$ source venv/bin/activate
$ (venv) jupyterhub # it works because my cylc-ui for the infinite-tree branch is located at ../cylc-ui, the default static folder! Cylc UI Note 1: One of the two browser sessions must be in incognito. Cylc Flow workflowsI use the following three workflows for testing. But you are free to use other workflows. fiveThis is my favorite workflow, for its simplicity and predicability.
families2This was an example Hilary posted in an issue on GitHub I believe, and it is quite simple too, with the difference it has a good and nested structure with families and tasks. Useful to validate the Tree view is displaying families (in the beginning it was only capable of displaying cycle points, tasks, and jobs).
complexThis workflow was my karma in 2019. Most problems that forced me to rewrite or drop parts of the initial code of the Tree view were caused by tests with this workflow. This is from the Cylc Flow 7.x examples I believe, but may have some modifications (there are further changes that could be applied, such as queues I think - Hilary might know - but I use the same as in the Cylc 7 branch I think). (code too long, see gist) one (mocked)Cylc UI comes with a mocked workflow, "one", that can be visualized with a command like While this workflow is static (this is a snapshot of a real workflow, recorded as JS for development), it is useful for testing, or for comparing the performance of components without networking, or without having to wait for things to be updated. It is possible to start two servers for
Another way to test without any network, is running |
Measuring performanceChromium performance tabI use the Chromium browser for tests normally. Without the extensions, and with no other tabs open (either in normal or incognito mode). Then clear any previous results, and press "Start recording and reload" (looks like a reload symbol). That should reload the page and initialize a performance benchmark of a few (4-5) seconds. During that time, it will measure several metrics, including JS Heap, number of listeners, time spent loading and evaluating scripts, as well as other things such as reflow, idle time, etc. Here's a screenshot of Cylc UI And the same workflow now with Cylc UI I have grouped by category. "Scripting" includes things like converting the GraphQL JS object to our tree object, and to the InfiniteTree data as well. While "Rendering" includes things like creating the canvas, painting, putting the elements in position and, in special, re-organizing the elements when there are changes in the DOM and the styles need to be updated. You may want to compare memory, number of event listeners, drill-in and inspect what other threads were doing during the test. Another useful test is to click "Record", then scroll down, and expand and collapse a few nodes (preferably a cycle point or family to test if the browser will cascade many events). What improvedDOM element hierarchy (See ref 1)
Before we had a recursive component, based on Vuetify tree, and on existing tutorials. While it worked, a change in the job details panel could trigger a reflow of its parent, and of its parents' parents, and so it goes. All the way up to the cycle point element, or the tree component element itself. Now each entry in the tree is a node, with a Less elements affected when recalculating styles - so less time spent as wellBelow is a comparison of "Rendering" and "Painting" for The same workflow, but for So while Less time spent "painting" renderingWith less elements, the browser also spends less time painting the screen. For While for About the same memory - but winner is still
|
Initial subjective tests in my environment with the complex suite (queue-limited to avoid killing my box with jobs) ... shows this performs pretty damned well 🎉 |
This comment has been minimized.
This comment has been minimized.
ca7189d
to
b1d5bb2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I haven't forgotten about this PR, just taking a while for me to get my head around it. Should get it signed off on Today or Monday. A couple of comments so far.
return [] | ||
} | ||
}, | ||
autoOpen: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought for later work, for default expansion/collapsing we could use:
- A maximum expand depth
- Which is what we currently do with Cylc7 [and in my Cylc Monitor PR] (i.e. expand the first N levels)*
- A function or list of conditionals.
- Perhaps slower but very powerful, would allow us to "filter" the display, fits into the infinite tree model well.
- e.g. collapse parameterisations by default.
- If nodes know their depth we could implement "maximum expand depth" in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before that would be a bit harder, as the collapse/extend state was controlled in the component.
The InifiniteTree
object keeps state in the data. So we have full control by simply choosing the initial/default state and setting the value in the JavaScript data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nodes know their depth we could implement "maximum expand depth" in this way.
Nodes do know their depth 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! But I should have explained better. I think I know one case that doesn't translate directly from the GraphQL results to the Tree view.
I don't remember how we are using depth (i.e. don't remember which nodes in our schema have it). But say we have a workflow with depth=0, then the next step would be cycle points with depth=1 in the tree.
But in our schema, I think the cycle point has a different depth?
We also have special cases, like families. I am not sure if the depth in families is using the correct hierarchy that we need for the tree—which is good I think; our GraphQL schema is independent of component.
Another corner case is the root
family, which is hidden by default. So the depth has to be corrected for that. So in the end, we need the depth
matching what we want to display in the Tree component, which is too specific for the component I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember how we are using depth (i.e. don't remember which nodes in our schema have it). But say we have a workflow with depth=0, then the next step would be cycle points with depth=1 in the tree.
Depth is defined on all nodes except jobs, and based on Family hierarchy.. Cycle points in the tree view are really the root
family in my mind... (BTW - maxDepth
is a field under workflow
if it helps with working out minimum margins or something)
src/styles/cylc/_tree.scss
Outdated
line-height: 1.8em; | ||
|
||
.node-data { | ||
margin-left: 12px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use em
for margin-left
(and also for padding-left
in the infinite tree code)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, will have to look with more calm why I used 12px
. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a blocker or anything just a quick comment, because the line-height
is set in em
above.
This comment has been minimized.
This comment has been minimized.
Thanks, this one was clearly an epic effort 🚀 |
This comment has been minimized.
This comment has been minimized.
Build passing on GitHub. Happy to report that it was easier than expected! Worked locally on "offline" mode, with dummy data. Also worked with no issues serving two workflows (five & families2). I used the branch of the pull request that fixes websockets in cylc-uiserver. Tested on Ubuntu with Chrome & Firefox. No issues. Also tested on Windows 10Pro with IE Edge. No issues. It appears to be a bit slower to render the infinite scroll when scrolling very fast, but could be that it was that way before too. It does look very similar to facebook & instagram when you scroll very fast. Also fixed two possible memory leaks, where data in a map and event listeners were not properly cleaned up. Ready for review again 👍 |
Would be nice to use a skeleton for scrolling, for future work...
Code looks good. Unfortunately the tree is rendering really slowly for me with the complex suite - even when held (~5s when I press Page-Down). Last time I tested it the complex suite was rendering quickly. Are you seeing different performance. I also got Opera using >2GiB memory which triggered the debugger to hold the session (which may or may not be related to the infinite tree): Sorry :( |
Thanks for testing it @oliver-sanders ! I may have introduced some bug after rebasing it, or maybe using Lumino with it needs some further work.
Good idea! There's a Vuetify component for that too: https://vuetifyjs.com/en/components/skeleton-loaders |
Hmmm, I agree - sorry Bruno! - something seems to have gone horribly wrong with the performance on this branch (just tested on the complex suite) |
@hjoliver I wonder if accessing the Tree view directly is also not working? From the Workflows Table, there should be a link to view the tree view I think. |
The table rows now link to the lumino tabs view, with the default tree view tab. (I did that last week). Is there a direct URL I can use to the tree view component (I know there is for the graph view.) |
Sorry, my environment is slightly broken while I finish updating some deps. Just had a look at Maybe if you copy the |
…eleton now being used
When the infinite tree was first implemented, the structure of the tree was different than what we have now. That's because we had several changes to the tree, such as fixing element width and how the elements react when the user re-sizes the browser (overflow, word-break, etc). We also had improvements to how the elements were displayed, how many events were triggered, and other changes for the deltas. This caused this pull request to actually break some of these changes/improvements. This commit should fix that, though tests may still need updating. But the tree functionality is back to what it was, plus the infinite tree.
It will be created in the data section, and then populated when the watcher is notified about updates (acts eagerly/immediate)
…ed in this Vue port
When the user navigates to the view, we will have a Tree view, and also a deltas subscription. If the user adds more tree views, we will keep using a single subscription. If the user removes all tree views, we will stop the subscription. If the user leaves the page, we will destroy the subscription. Finally, if the user changes the workflow, we destroy the subscription, and create a new one, also adding a new tree view as that is the default.
Rebased |
Fair enough, makes sense.
Ah so it does, and the style seems to hold together perfectly. So long as accessibility is fine I'm happy, looks good to me: Sorry, but I've found another bug :( If I expand items in the tree, then scroll up or down, sometimes items get cut off of the front or end of the tree. I can replicate this with the "one" suite in the mocked data: Expand/collapse or switching tab seems to be enough to un-stick the tree. I guess this sort of thing is always a risk with a virtual tree so it might be very difficult to fix this entirely. If we could reduce the likelyhood, and make it clearer to the user that there is content that they can't see (e.g. with a skeleton for the missing items) that would be enough. |
Ah, dang good spot, I guess the expand/collapse state is stored in the shared tree rather than locally in the view instance. Might be related to the change from JS to CSS logic? |
I've played around quite extensively with the complex suite and a smaller suite. So far I've seen this all the time in a second tab (if I have a second tab open) but never in the first (or only) tab. If I have a second tab open, the view never updates on scrolling. If I scroll the most of the rendered items off-screen, then expand or collapse one of the remaining nodes, at that point the view in the tab updates (but scrolling more results in the same problem again). |
(Scrolling performance seems really good 👍 ) |
I tested with one and multiple workflows, and with tree, and tree+graph. But never tested tree+tree 😁 good catch @oliver-sanders ! And @hjoliver is right about the expand/collapse not being independent. There is a single tree, backing both tree components. When you expand one, it emits an event telling the infinite tree that we want to expand a node. The node's state is changed, reflecting the new Both issues reported here are due to this state-sharing issue. When you expand on one tree component, the other component receives the new state, and changes the expand/collapse icon. However, the elements are not rendered. So if you have a limited viewport, with scroll active, and expand one one component, the other will change its state without rendering the elements. When you scroll on the other window, the component will think it needs to show the children elements of the expanded node, but as these elements were not rendered, you see only a blank area. I believe if we are able to tell both components to change the state, and render the newly expanded nodes, it should work correctly. Then we can discuss having separate states per widget... or maybe having a toggle button to link the widgets. |
Actually there's one more issue. Related to the double scroll bars. If you scroll up or down, and you have the view area scroll, and also the component scroll, both active. Then looks like the outer scroll bar needs to emit an event to tell the tree we need to refresh it. Without that happening, you can end up with empty areas in the tree, due to elements not being rendered. We already have some code taking care of events when you add other widgets, to tell our tree we want it to update. It might be just a matter of finding what else we need to listen to and tell the tree to update. p.s. the tree component uses vue-virtual-scroller, which uses the intersection observer API, which has a few issues with browsers. It could be one of these issues too 🤓 |
Scrollbar issueHere's how I'm troubleshooting this first issue. First I resized the browser view port area so that I had both scroll bars visible, and successfully reproduced the issue (chrome/chromium/firefox on Linux - i.e. not a browser issue). Then I used VueDevTools to find the
These are the three methods that I found in the object that fixed it. The The WIP WorkaroundIf this issue is too complicated to be fixed, one possible workaround is to support only users with a viewport large enough to display the widgets without scroll bar (i.e. support at least the minimum height of the widgets, plus toolbar and padding/margin) |
shared state (expand/collapse) issueThe
If we have two or more Lumino widgets added to the Workflow view, each widget will have a When you expand or collapse a node, we call the If you have two infinite trees, But only I think we have a few possible solutions so far:
1. call
|
Closed as per Riot message 🎉
With agreement from the UK end, via oliver sanders , we are going to shelve the infinite tree for now.
|
These changes close #330, and close #346, and close #256 (no recursion - no need for so many events, it's now controlled by the
infinite-tree
library), and close #227 (memory is about the same initially, but doesn't skyrocket over time), and close #222Now also closes #403 , as I used it to improve the JS code, and avoid having to build cycle points by looking at all the families (thanks to @dwsutherland 's tip).
Reverts #232 because the offline mode uses
cylc client graphql
to fetch data, and it doesn't get the__typename
, which is set only by the JS ApolloClient.Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.Pending
try to add the Infinite Tree as a separate component, instead of replacing the current Tree, and use as an extra Lumino widget