Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[orchestra] extract graph logic and enhance with cycle detection + coloring #5614

Merged
merged 18 commits into from
Jun 6, 2022
Merged

[orchestra] extract graph logic and enhance with cycle detection + coloring #5614

merged 18 commits into from
Jun 6, 2022

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented May 30, 2022 •
edited
Loading

  • extract graph logic
  • cycle detection (feature gated, NetworkBridge will always cause a cycle now)
  • highlight cycles (does not care about bounded or unbounded, that would require additional annotations)
  • add noone-sends and noone-consumes dummy nodes

@drahnr drahnr added the A3-in_progress Pull request is in progress. No review needed at this stage. label May 30, 2022
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 30, 2022
@drahnr drahnr added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 30, 2022
@drahnr
Copy link
Contributor Author

drahnr commented May 30, 2022

image

adds two extra nodes, unconsumed (skull) and never sent (fountain)

@drahnr
Copy link
Contributor Author

drahnr commented May 30, 2022 •
edited
Loading

Coloring/annotating cycles properly is pending, the logic is already there:

   Compiling orchestra v0.0.1 (/media/supersonic1t/projects/parity/polkadot/node/orchestra)
   Compiling polkadot-node-subsystem-types v0.9.23 (/media/supersonic1t/projects/parity/polkadot/node/subsystem-types)
   Compiling polkadot-overseer v0.9.23 (/media/supersonic1t/projects/parity/polkadot/node/overseer)
>>> Found 8 cycles <<< 
Wrote dot graph to /media/supersonic1t/projects/parity/polkadot/target/debug/build/orchestra-proc-macro-d0a92225d8323089/out/overseer-subsystem-messaging.dot
    Finished dev [unoptimized + debuginfo] target(s) in 4.66s

@drahnr drahnr self-assigned this May 30, 2022
drahnr added 4 commits May 31, 2022 10:00
@drahnr
avoid some clones
a60a3ba
@drahnr
better doc
841792d
@drahnr
maybe better to use ✨
7ec0f15
@drahnr
print all cycles, or an error
3827977 
Currently kosaraju_scc returns invalid cycles, which is yet to be investigated.
@drahnr
Copy link
Contributor Author

drahnr commented May 31, 2022

image

drahnr added 6 commits May 31, 2022 13:55
@drahnr
don't print tiny cycles, if there is nothing to unvisited anymore, th…
116f79f 
…ere is a cycle
@drahnr
make print better
1dbda9e
@drahnr
fmt ffs
92cb119
@drahnr
correct: cycle -> scc
b8e875d 
A strongly connected cluster contains at least one cycl, but could include more.
So this should be distringuished in the implementation to avoid some confusion.
@drahnr
fix loop exit condition
372a167
@drahnr
add a test for kosaraju behavior
b98b419
@drahnr drahnr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 1, 2022
@drahnr drahnr requested review from sandreim, ordian and tdimitrov June 1, 2022 11:32
@drahnr drahnr removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 1, 2022
@drahnr
Copy link
Contributor Author

drahnr commented Jun 1, 2022

image

with the output of:

⚡ Found 1 strongly connected cluster which includes at least one cycle
cycle(001) ∈ α: NetworkBridge ~~{"GossipSupportMessage"}~~> GossipSupport ~~{"NetworkBridgeMessage"}~~>  *
Wrote dot graph to /media/supersonic1t/projects/parity/polkadot/target/debug/build/orchestra-proc-macro-33f2b73e449a7593/out/overseer-subsystem-messaging.dot

(pre splitting the network bridge #5616 )

ordian
ordian approved these changes Jun 3, 2022
View reviewed changes
@drahnr drahnr requested a review from eskimor June 3, 2022 13:54
sandreim
sandreim approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

LGTM. It would also be really helpful to have the option to create a graph only with only the cycle.

@drahnr
Copy link
Contributor Author

drahnr commented Jun 6, 2022

I'll create a separate PR that will add these graphs of cycles only

@drahnr
Copy link
Contributor Author

drahnr commented Jun 6, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b21d5df into master Jun 6, 2022
@paritytech-processbot paritytech-processbot bot deleted the bernhard-orchestra-extract-graph-logic branch June 6, 2022 16:35
@sandreim sandreim mentioned this pull request Jun 9, 2022
Closed
al3mart pushed a commit that referenced this pull request Jul 14, 2022
@drahnr
[orchestra] extract graph logic and enhance with cycle detection + co…
bb81181 
…loring (#5614)

* extract graph

* move into scope

* remove dead code

* add special nodes

* avoid some clones

* better doc

* maybe better to use ✨

* print all cycles, or an error

Currently kosaraju_scc returns invalid cycles, which is yet to be investigated.

* don't print tiny cycles, if there is nothing to unvisited anymore, there is a cycle

* make print better

* fmt ffs

* correct: cycle -> scc

A strongly connected cluster contains at least one cycl, but could include more.
So this should be distringuished in the implementation to avoid some confusion.

* fix loop exit condition

* add a test for kosaraju behavior

* unify on 'component'

* disable graph by default

https://github.com/paritytech/ci_cd/issues/433

* chore: fmt

* move graph only to graph_helpers
@sandreim sandreim mentioned this pull request Aug 16, 2022
Closed
@drahnr drahnr mentioned this pull request Sep 27, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian approved these changes

@sandreim sandreim sandreim approved these changes

@tdimitrov tdimitrov Awaiting requested review from tdimitrov

@eskimor eskimor Awaiting requested review from eskimor

Assignees

@drahnr drahnr

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drahnr @ordian @sandreim