Optimizations for CompareRoutePolicies

[millstein]

A few performance optimizations for CompareRoutePolicies:

• When identifying differences between two BDDRoutes, ignore the input constraints at first; only impose them later on specific differences that are found (rather than on the entire BDDRoute as done previously)
• Don't compute the intersection between two path conditions unless it is actually needed

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #8944 (326212c) into master (3e4e8e3) will decrease coverage by 0.04%.
Report is 3 commits behind head on master.
The diff coverage is 97.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8944      +/-   ##
==========================================
- Coverage   72.53%   72.50%   -0.04%     
==========================================
  Files        3323     3323              
  Lines      169603   169626      +23     
  Branches    19915    19914       -1     
==========================================
- Hits       123030   122995      -35     
- Misses      37410    37465      +55     
- Partials     9163     9166       +3     

Files	Coverage Δ
...ompareroutepolicies/CompareRoutePoliciesUtils.java	72.18% <100.00%> (+3.11%)	⬆️
...java/org/batfish/common/bdd/MutableBDDInteger.java	62.20% <80.00%> (-2.26%)	⬇️

... and 7 files with indirect coverage changes

[nickgian]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @anothermattbrown and @millstein)

a discussion (no related file):
how do we evaluate these changes? We should test them against a few route-maps to ensure they help.

---

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 326 at r1 (raw file):

          break;
        case ADMIN_DIST:
          result = r1.getAdminDist().allDifferences(r2.getAdminDist());

what changed in all of these cases and we need the extra constraint? Was the old implementation wrong or is something done differently now?

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 423 at r1 (raw file):

    List<Tuple<Result<BgpRoute>, Result<BgpRoute>>> differences = new ArrayList<>();

    BDDFactory factory = JFactory.init(100000, 10000);

I’ve seen cases where the size of node-table was doubled 11 times, should we pick a larger number here (e.g., 1M)?

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 435 at r1 (raw file):

    List<TransferReturn> otherPaths = computePaths(otherTBDD);

    // Potential optimization: if a set of input routes between the two paths is the same then we

Not related to the changes in this CR, just trying to think what would be a more useful variant of this optimization in the comment.

Could we also build the union of every inputRoutesOther we found to be part of a difference, and exit the inner loop early when that union is equal to inputRoutes?
i.e.:

  for (path : paths) {
    BDD union = BDD.ZERO;
    for (otherPath: otherPaths) {
         …
         // after andSat code
         union = union.or(inputRoutesOther);

         …. // after computing differences at the end of the loop
         if union.equals(inputRoutes) { break; }   
    }

That might save you from comparing with all other paths, e.g., in case you add an extra condition/stanza somewhere in your route-map. It won’t work if you remove a stanza…
Anyway, just sharing in case it sparks a better idea for you.

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 480 at r1 (raw file):

                if (!outputConstraints.isZero()) {
                  behaviorDiff = true;
                  finalConstraints = intersection.and(outputConstraints);

you can have counterExampleOutputConstraints return the intersection directly (or Zero) to slightly simplify this.

[millstein]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anothermattbrown and @nickgian)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 326 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

what changed in all of these cases and we need the extra constraint? Was the old implementation wrong or is something done differently now?

The old implementation was wrong. For example, suppose one route map sets the administrative distance to the value 0 and accepts, while the other route map simply accepts. We would previously say there's a difference but the example we provide wouldn't show a difference, since it will still use the default admin distance of 0 in the input route. The above constraint forces it to choose a different value like 1, so that the difference is exposed. I added a test case.

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 423 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

I’ve seen cases where the size of node-table was doubled 11 times, should we pick a larger number here (e.g., 1M)?

I don't have a good sense. 100,000 is the largest being used currently in the project, as far as I can tell. Maybe @anothermattbrown has a thought?

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 435 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

Not related to the changes in this CR, just trying to think what would be a more useful variant of this optimization in the comment.

Could we also build the union of every inputRoutesOther we found to be part of a difference, and exit the inner loop early when that union is equal to inputRoutes?
i.e.:

  for (path : paths) {
    BDD union = BDD.ZERO;
    for (otherPath: otherPaths) {
         …
         // after andSat code
         union = union.or(inputRoutesOther);

         …. // after computing differences at the end of the loop
         if union.equals(inputRoutes) { break; }   
    }

That might save you from comparing with all other paths, e.g., in case you add an extra condition/stanza somewhere in your route-map. It won’t work if you remove a stanza…
Anyway, just sharing in case it sparks a better idea for you.

Yes, that's a good thought. I tried something somewhat related, which is that if you swap the order of the for loops and otherPath implies path, then you don't need to compare otherPath against any more paths in paths since they must be disjoint, so you can break out of the inner loop. But at least in the cases I was looking at, that didn't help much -- you still need to do O(n^2) intersections (and I think the same is true of the above optimization). Further, it turned out that a much bigger bottleneck was the cost of handling two paths that do intersect, rather than avoiding unnecessary path intersections, so I ended up focusing on the former.

[nickgian]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @millstein)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 326 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

The old implementation was wrong. For example, suppose one route map sets the administrative distance to the value 0 and accepts, while the other route map simply accepts. We would previously say there's a difference but the example we provide wouldn't show a difference, since it will still use the default admin distance of 0 in the input route. The above constraint forces it to choose a different value like 1, so that the difference is exposed. I added a test case.

thanks for the explanation and adding a test.

[dhalperi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @millstein)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/compareroutepolicies/CompareRoutePoliciesUtils.java line 423 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

I don't have a good sense. 100,000 is the largest being used currently in the project, as far as I can tell. Maybe @anothermattbrown has a thought?

It doesn't really matter. In BDDPacket we use 1M:

batfish/projects/batfish-common-protocol/src/main/java/org/batfish/common/bdd/BDDPacket.java

Line 41 in 6709c01

private static final int JFACTORY_INITIAL_NODE_TABLE_SIZE = 1_000_000;

[dhalperi]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @millstein)

[millstein]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @anothermattbrown)

a discussion (no related file):

Previously, nickgian (Nick Giannarakis) wrote…

how do we evaluate these changes? We should test them against a few route-maps to ensure they help.

done

---