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

APRProof get_steps optimizations #4585

Merged
merged 6 commits into from
Aug 14, 2024
Merged

Conversation

nwatson22
Copy link
Member

@nwatson22 nwatson22 commented Aug 13, 2024

  • Calling nonzero_depth is (relatively) expensive, when it's done for each pending nodes and we have 50+ pending nodes. This is because it traverses the KCFG to find all paths between the init node and the current node.
  • We were calling nonzero_depth twice unnecessarily. Caches the result in a variable instead.
  • In most cases, depth will be immediately obvious to be nonzero just from looking at the first edge on the path. In this case, skips the traversal. Also skips the traversal if the nodes we are checking the if the depth between is 0 for if they are the same node.
  • Tested on test%kontrol%VetoSignallingTest.testVetoSignallingInvariantsArePreserved4(uint256,uint256,uint256) from the lido tests. Around 400 nodes in, the sync time is 0.3-0.4 seconds without the optimization. With the optimization it is around 0.015 to 0.04 seconds (although this also depends on the number of pending nodes at any given time). This seems to eliminates the bulk of the sync time when there are a large number of pending nodes (~30+).

@nwatson22 nwatson22 self-assigned this Aug 13, 2024
@rv-jenkins rv-jenkins changed the base branch from master to develop August 13, 2024 20:13
# Short-circuit and don't run pathing algorithm if there is no 0 length path on the first step.
node_1_successors = self.successors(_node_1_id)
node_2_successors = self.successors(_node_2_id)
path_lengths = [self.path_length([successor]) for successor in node_1_successors + node_2_successors]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could inline the node_1_successors definitions here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@PetarMax PetarMax marked this pull request as ready for review August 14, 2024 18:31
@rv-jenkins rv-jenkins merged commit df56618 into develop Aug 14, 2024
17 checks passed
@rv-jenkins rv-jenkins deleted the noah/zero-depth-optimization branch August 14, 2024 19:02
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.

3 participants