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

GScan improvements, propagate state #736

Closed
wants to merge 16 commits into from
Closed

Conversation

kinow
Copy link
Member

@kinow kinow commented Aug 23, 2021

These changes close #721 and supersede #733 (not yet, but will in a few more commits 🤞 )

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow kinow marked this pull request as draft August 23, 2021 02:47
@kinow
Copy link
Member Author

kinow commented Aug 23, 2021

While working on #733 for #721, I noticed the computed variable that would re-trigger more elements in the UI to be updated than desired.

In this pull request I will move the GScan data out of the component, into Vuex, and update it incrementally based on deltas. The goal is to have the least amount of elements being updated as possible.

Here's a screenshot from this morning where I hacked the code to print whenever a TreeItem was updated.

Screenshot from 2021-08-23 10-50-06

Screenshot from 2021-08-23 10-50-32

Even though five was being updated, the TreeItem's for jh1, rose/run1, etc, were being updated as well 😬 which results in a major performance issue.

import Alert from '@/model/Alert.model'
import applyDeltasTree from '@/components/cylc/tree/deltas'

const state = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've split the workflows.module since it had data used by GScan, and also by the Tree view. The tree view data is relatively more complex at this point, so moving it out simplifies the workflows.module.

* @param {Object.<String, Object>} lookup
* @return {Result}
*/
function applyDeltasAdded (added, lookup) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code added here was in workflows/deltas.js, but was used only by the Tree view. Instead of moving to the tree view, I've moved it here to common since if we ever need to use a delta that creates a lookup, anyone can simply call this function ☝️

I think I won't need a lookup for GScan, at least for now. Or at least I am avoiding adding one unless really needed. But if I need to add one, then I'll use this function.

@kinow
Copy link
Member Author

kinow commented Aug 24, 2021

I've implemented most of what I had in mind for GScan. But this afternoon just before a meeting I decided to look if the TreeItem's were still being updated, and strangely everything was still updating.

I created a smaller test scenario, with yarn run serve and only 1 workflow one. Then I push a delta manually to have another worklfow a. This workflow is sibling of one in the GScan tree, but one is being updated too, which is not really desirable.

I'm investigating now why that's happening, and so far these are the two best leads: vuejs/vue#7257 & vuejs/vue#10115 (comment) since they match what why-did-you-render is reporting in the browser console... more to come 👍 🛏️

@kinow kinow force-pushed the gscan-improv branch 2 times, most recently from 951c0e1 to ee607a7 Compare August 31, 2021 21:40
@kinow
Copy link
Member Author

kinow commented Sep 6, 2021

All right, coming back to this one after lunch, and strong ☕ to help me fixing conflicts (no conflicts yet 🎉 ), and remember where was I.

@kinow kinow force-pushed the gscan-improv branch 2 times, most recently from 17dd08d to aa8843e Compare September 7, 2021 23:25
@kinow
Copy link
Member Author

kinow commented Sep 8, 2021

Hmm, I think I figured how to store the data in each node in a way that I can update with added/updated/pruned deltas.

The leaf nodes in the GScan tree are workflows. They will keep the same structure (latestStates + stateTotals). The intermediary nodes (workflow-name-part nodes, as I called them 🧐 ) will have a computed variable with same name. This way the component's templates won't need to be changed.

However, the real data will hold:

  • maps/dictionaries backing the latestStates and stateTotals computed variables, where
    • the key is the leaf node ID
    • the value is the leaf node latestStates or stateTotals

Then the computed variables will display the summarized numbers of latestStates and stateTotals for all entries in the maps/dictionaries.

This is necessary so that we can:

  1. add the values to the dictionary/map (deltas.added),
  2. update the existing values (deltas.updated), and
  3. remove the values when the node appears in a deltas.pruned

The approach of keeping a pointer to the leaf-node stateTotals/latestStates is necessary so that when a leaf-node is pruned, we can safely delete it from the map/dictionary, causing the computed variables to be calculated again.

An alternative would be to store the ID's only. And then use the lookup to locate the node, and finally retrieve its latestStates and stateTotals. Will think on the pros & cons of each alternative, and try to come up with some code.

At the moment, the code in this pull request correctly produces a GScan tree with added nodes, with no stateTotals/latestStates. It's pending:

  • refactor GScan using tree lookup
  • performance and code-review improvements (found a few things that were unnecessary, or could be improved)
  • add nodes with deltas.added to the GScan tree using a tree/lookup approach as in the tree component
  • update nodes with deltas.updated
  • prune nodes with deltas.pruned
  • create a WorkflowStateSummary component to isolate the functionality for task summary state in a workflow
  • sort the nodes again after an update (e.g. you have workflows a/run1 and a/run2, both stopped; you start a/run2, now it must move to the top)
  • propagate states

@kinow kinow force-pushed the gscan-improv branch 2 times, most recently from cf0c1b5 to f3a81df Compare September 10, 2021 05:48
@kinow
Copy link
Member Author

kinow commented Sep 10, 2021

Done for the day, next week will start sorting the parent.children of a node when it was updated. Noticed this error when I started a random run and it didn't move up the tree.

Previously, we were always re-generating the whole tree 😨 so it looked like the tree element was correctly re-positioned… but it was actually due to the new three being created from scratch.

@kinow kinow force-pushed the gscan-improv branch 5 times, most recently from c2c4972 to de31daa Compare September 14, 2021 02:27
@kinow
Copy link
Member Author

kinow commented Sep 14, 2021

Think I found how to improve GScan a bit more, preventing it from re-rendering all the nodes in the GScan tree, refs:

@kinow kinow force-pushed the gscan-improv branch 2 times, most recently from 1d19a63 to d552e90 Compare September 19, 2021 22:38
@kinow
Copy link
Member Author

kinow commented Sep 20, 2021

Will edit the last commit tomorrow, but wanted to save what I wrote today. Pending more manual tests with working workflows. Here's a preview of what it's looking like in the offline mode right now (modified one workflow a little, will undo tomorrow since it's used by e2e tests).

GIFrecord_2021-09-20_164611

@kinow kinow force-pushed the gscan-improv branch 2 times, most recently from 230bc61 to fc4c584 Compare September 23, 2021 02:24
@kinow kinow self-assigned this Oct 4, 2021
@kinow
Copy link
Member Author

kinow commented Oct 19, 2021

This PR can probably be closed.

I started working on the GScan here to propagate states. Then realized we had some improvements that could be applied to GScan, and started changing that here.

Then another PR came that changed the callbacks and deltas in GScan 😬

I moved the improvements to #802 , mimicking with we have in the Tree view.

There's one commit in this PR that tries to propagate the states in GScan. I think someone could either drop the other commits and fix conflicts after #802 has been merged, and then finish the current work.

Or just close this and use as reference for future implementations.

🖖

@hjoliver
Copy link
Member

Thanks for the advice @kinow, good to know 👍

@wxtim
Copy link
Member

wxtim commented Jun 23, 2023

Stale.

@wxtim wxtim closed this Jun 23, 2023
@MetRonnie MetRonnie removed this from the Pending milestone Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate summary state up the tree in GScan
5 participants