fix: Prevent O(2^N) exponential blowup in diamond DAG manifest validation#1990
fix: Prevent O(2^N) exponential blowup in diamond DAG manifest validation#1990peekmoar wants to merge 1 commit into
Conversation
|
Well, it's the same root cause (diamond DAGs), but a different code path. #1887 fixed exponential memory growth in the manifest reading/construction path - the reader.rs/store deserialization stuff. This PR fixes exponential time spent in the validation path. It's specifically the This fix is need I think. It's a separate recursive walk entirely. It's probably something that could be solved by a timeout as well but I think this way is more correct. Thank you for taking a look. I'm going to try closing and opening for the Adobe CLA |
peekmoar
force-pushed
the
fix/diamond-dag-dedup-exponential-blowup
branch
from
ab3b083 to
240ea33
Compare
|
Not rushing you but is there anything else that I need to do to get this change in? |
peekmoar
force-pushed
the
fix/diamond-dag-dedup-exponential-blowup
branch
from
7b385e0 to
2bbb621
Compare
| let elapsed = start.elapsed(); | ||
|
|
||
| let manifest_count = reader.iter_manifests().count(); | ||
| assert!(manifest_count > 0, "should have parsed manifests"); |
There was a problem hiding this comment.
How many manifests should be parsed? Checks could be tightened here to make sure result stays the same even if code changes in future
There was a problem hiding this comment.
Also: It may be good to verify a Reader reports all expected checks in reader.validation_results()
| /// Test that a diamond DAG at depth 8 completes in reasonable time. | ||
| /// | ||
| /// Without the dedup fix, depth 8 would cause 2^8 = 256 manifest visits. | ||
| /// With the fix, it should visit only ~17 unique manifests. |
There was a problem hiding this comment.
Why the approximation? It should always be the same number at each run?
|
|
||
| // With the dedup fix, this should complete in well under 60 seconds. | ||
| // Without the fix at depth 8, it would take significantly longer due to 256 visits. | ||
| assert!( |
There was a problem hiding this comment.
I would prefer not to have timed unit test. If we know this code works you can remove this test.
|
I am OK with the changes. Please address the PR comments and I will approve. |
3 participants
Summary
get_claim_referenced_manifests_impl: checksvi.manifest_map.contains_key()before recursing (it already tracks visited manifests)ingredient_checks/ingredient_checks_async: addvisited: &mut HashSet<String>parameter with early-exit viavisited.insert()Test plan
test_diamond_dag_dedup.rsthat constructs a depth-8 diamond DAG and verifies it completes within 60 seconds (takes ~1.5s with the fix)cargo checkpassescargo test --test test_diamond_dag_deduppasses