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

Optimizations for write_cfg_data #4569

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Conversation

nwatson22
Copy link
Member

@nwatson22 nwatson22 commented Aug 2, 2024

Makes progress in fixing the slowdown as proofs get large.

I'll call the "sync time" the time in between receiving a step result and starting to wait for the next step result on the master thread.

As proofs get more and more nodes, calling to_dict() on every node gets progressively harder and this was taking up a substantial portion of the sync time as proofs got to 500+ nodes.

Instead of generating the entire KCFG dict with KCFG.to_dict() before passing this to KCFGStore.write_cfg_data(), which discards all of this work anyway for the final product and replaces it with just a list of the node IDs, it is significantly faster to just pass it the list of the node IDs and have it use the KCFG object, which already has a dictionary storing nodes by ID to find the vacuous and stuck nodes, and to get the newly created nodes. This way we can call to_dict() on each node only when it is first created.

To give an idea of a benchmark, I used a LoopsTest.test_sum_1000() (linear and long-running) with --max-depth 1 and --max-iterations 1000. Before this change it gets to 1000 iterations in 59:06, and after this change it does it in 40:31. Before the change the sync time as this proof approached 1000 nodes ranged between about 3.4 to 4.2 seconds. After the change ranged from about 1.39 to 1.54 seconds.

The big remaining chunk of sync time when the proof gets large seems to be in get_steps().

@nwatson22 nwatson22 self-assigned this Aug 2, 2024
@rv-jenkins rv-jenkins changed the base branch from master to develop August 2, 2024 20:06
@nwatson22 nwatson22 requested review from ehildenb and PetarMax August 2, 2024 23:29
@PetarMax
Copy link
Contributor

PetarMax commented Aug 5, 2024

Before I forget, I think that it would be a good idea to have the set of pending nodes made explicit and maintained as they are modified - this should introduce a major speed-up since get_steps() looks for pending nodes, which means a traversal and analysis of the entire KCFG.

@PetarMax
Copy link
Contributor

PetarMax commented Aug 5, 2024

This looks like a good improvement, but I think that we might need to assess on engagement code. I am testing it with intermittent writing to disk (every max-frontier-parallel iterations), and the speed-up is (expectedly) greater than the one shown here. Perhaps we could do both?

@nwatson22
Copy link
Member Author

nwatson22 commented Aug 5, 2024

@PetarMax Thanks, that is a good suggestion for how we can solve the performance issues with the APRProof implementation of get_steps().

Between the intermittent writing to disk and this change I suspect that the two would eat into each other's performance gain, but it sounds from what you're saying they stack at least somewhat. I will try to test this and #4563 together as well.

Edit: I think I misunderstood. The performance gain on the engagement code from this PR that I tested was also greater than what I wrote here about the test I ran from kontrol's test suite, although I don't know the exact reason why. Maybe it's just because the individual node JSONs are larger for lido proofs. But I will still test and see if these two techniques stack.

@PetarMax
Copy link
Contributor

PetarMax commented Aug 6, 2024

This looks good to me. @ehildenb, @tothtamas28?

@PetarMax PetarMax marked this pull request as ready for review August 8, 2024 10:17
@PetarMax PetarMax merged commit d55c98f into develop Aug 8, 2024
17 checks passed
@PetarMax PetarMax deleted the noah/cfg-writing-optimization branch August 8, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants