AclIpSpace: flatten if possible during union (#3609)

Makes `union(union(a, b), c)` equivalent to `union(a, b, c)` instead of nested. Concerns that this may hurt forwarding analysis caching, but in practice we've seen little difference, maybe improvement, in computation time.
batfish · Apr 15, 2019 · aa63673 · aa63673
1 parent 62d18b7
commit aa63673
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 34 deletions.
diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AclIpSpace.java b/projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AclIpSpace.java
@@ -217,6 +217,26 @@ public static Builder rejecting(Iterable<IpSpace> ipSpaces) {
     return union(Arrays.asList(ipSpaces));
   }
 
+  /**
+   * When an {@link AclIpSpace} is a pure union (list of only permit lines), flattens it into the
+   * permitted spaces.
+   *
+   * <p>This function makes {@code union(union(a, b), c)} equivalent to {@code union(a, b, c)}.
+   */
+  private static Stream<IpSpace> flattenAclIpSpacesForUnion(IpSpace space) {
+    if (!(space instanceof AclIpSpace)) {
+      return Stream.of(space);
+    }
+    AclIpSpace aclSpace = (AclIpSpace) space;
+    List<AclIpSpaceLine> lines = aclSpace.getLines();
+    if (lines.stream().allMatch(l -> l.getAction() == LineAction.PERMIT)) {
+      // This is just a big union, flatten it into the list of spaces it unions.
+      return lines.stream().map(AclIpSpaceLine::getIpSpace);
+    }
+    // Not a pure union, so don't flatten.
+    return Stream.of(aclSpace);
+  }
+
   /**
    * Set-theoretic union of multiple {@link IpSpace IP spaces}.<br>
    * {@code null} ipSpaces are ignored. If all arguments are {@code null}, returns {@code null}.
@@ -225,6 +245,7 @@ public static Builder rejecting(Iterable<IpSpace> ipSpaces) {
     IpSpace[] nonNullSpaces =
         StreamSupport.stream(ipSpaces.spliterator(), false)
             .filter(Objects::nonNull)
+            .flatMap(AclIpSpace::flattenAclIpSpacesForUnion)
             .toArray(IpSpace[]::new);
     if (nonNullSpaces.length == 0) {
       // no constraint

diff --git a/projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/AclIpSpaceTest.java b/projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/AclIpSpaceTest.java
@@ -53,6 +53,15 @@ public void testUnion() {
     assertThat(union(EmptyIpSpace.INSTANCE, ipSpace), equalTo(ipSpace));
   }
 
+  @Test
+  public void testUnionNested() {
+    IpIpSpace a = Ip.parse("1.2.3.4").toIpSpace();
+    IpIpSpace b = Ip.parse("1.2.3.5").toIpSpace();
+    IpIpSpace c = Ip.parse("1.2.3.6").toIpSpace();
+    assertThat(union(a, b, c), equalTo(union(union(a, b), c)));
+    assertThat(union(a, b, c), equalTo(union(a, union(b, c))));
+  }
+
   @Test
   public void testStopWhenEmpty() {
     IpSpace space =

diff --git a/tests/parsing-tests/unit-tests-nodes.ref b/tests/parsing-tests/unit-tests-nodes.ref
@@ -20491,23 +20491,15 @@
                         {
                           "action" : "PERMIT",
                           "ipSpace" : {
-                            "class" : "org.batfish.datamodel.AclIpSpace",
-                            "lines" : [
-                              {
-                                "action" : "PERMIT",
-                                "ipSpace" : {
-                                  "class" : "org.batfish.datamodel.IpWildcardIpSpace",
-                                  "ipWildcard" : "1.2.3.4"
-                                }
-                              },
-                              {
-                                "action" : "PERMIT",
-                                "ipSpace" : {
-                                  "class" : "org.batfish.datamodel.IpWildcardIpSpace",
-                                  "ipWildcard" : "1.2.3.5"
-                                }
-                              }
-                            ]
+                            "class" : "org.batfish.datamodel.IpWildcardIpSpace",
+                            "ipWildcard" : "1.2.3.4"
+                          }
+                        },
+                        {
+                          "action" : "PERMIT",
+                          "ipSpace" : {
+                            "class" : "org.batfish.datamodel.IpWildcardIpSpace",
+                            "ipWildcard" : "1.2.3.5"
                           }
                         },
                         {
@@ -20597,23 +20589,15 @@
                           {
                             "action" : "PERMIT",
                             "ipSpace" : {
-                              "class" : "org.batfish.datamodel.AclIpSpace",
-                              "lines" : [
-                                {
-                                  "action" : "PERMIT",
-                                  "ipSpace" : {
-                                    "class" : "org.batfish.datamodel.IpWildcardIpSpace",
-                                    "ipWildcard" : "1.2.3.4"
-                                  }
-                                },
-                                {
-                                  "action" : "PERMIT",
-                                  "ipSpace" : {
-                                    "class" : "org.batfish.datamodel.IpWildcardIpSpace",
-                                    "ipWildcard" : "1.2.3.5"
-                                  }
-                                }
-                              ]
+                              "class" : "org.batfish.datamodel.IpWildcardIpSpace",
+                              "ipWildcard" : "1.2.3.4"
+                            }
+                          },
+                          {
+                            "action" : "PERMIT",
+                            "ipSpace" : {
+                              "class" : "org.batfish.datamodel.IpWildcardIpSpace",
+                              "ipWildcard" : "1.2.3.5"
                             }
                           },
                           {