HeaderSpace: use immutable structures internally #780

dhalperi · 2017-12-18T19:40:21Z

It looks we're spending an obscene amount of memory in empty TreeSet and ArrayList classes in HeaderSpace-related classes.

There are a few good ways to move forward:

Allow null for empty collections. Callers should wrap getX with null-checks. When they want to add an item to a set, they have to initialize.
- IMO this is an unfortunate amount of work for the caller.
Switch to empty/immutable collections internally. These are generally (much) more memory-efficient. Empty collections are in fact singletons Java-wide; and if we use the Guava classes, we'll likely save some copies when structures are copied/used in multiple places.
- we can copy-on-set, which also means we could relax the setX contract to accept un-sorted sets or even arbitrary collections.
- Callers only need to write annoying code where they used to modify the result of a getter: i.e.around getX().add(x) -> setX(...make a new collection containing getX and x).
- we could add addX forms that are internally equivalent to the above, but callers may have smarter logic (e.g., most of these are in the same function that allocates the interface)

I opted to go with the second.

This change is

Most of the memory in the process is taken up by empty TreeSets and the like. If we use Immutable/Empty data structures for these, we'll save a huge amount of RAM.

dhalperi · 2017-12-19T07:00:43Z

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Interface.java, line 378 at r1 (raw file):

  private List<SubRange> _allowedVlans;

  private SortedSet<Prefix> _allPrefixes;

Changed this to Sorted since it was initialized as a TreeSet and the refs broke otherwise.

Comments from Reviewable

dhalperi · 2017-12-19T07:08:52Z

Review status: 0 of 35 files reviewed at latest revision, 3 unresolved discussions.

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromDestinationPrefixList.java, line 45 at r1 (raw file):

                    } else {
                      return new IpWildcard(rfLine.getPrefix());
                    }

could add a helper getter to RouteFilterList to take over these ~10 lines of code, if useful. Used in 4 or 5 FwFrom rules.

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromFragmentOffset.java, line 28 at r1 (raw file):

    Set<SubRange> offsets = Collections.singleton(_offsetRange);
    if (_except) {
      line.setNotFragmentOffsets(offsets);

this was a set, not an add, so I kept it that way. Just noting the difference.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java, line 1210 at r1 (raw file):

      newIface.setPrefix(iface.getPrimaryPrefix());
    }
    newIface.setAllPrefixes(iface.getAllPrefixes());

does this not need the primary prefix? Cisco includes it.

Comments from Reviewable

dhalperi · 2017-12-19T07:09:37Z

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java, line 1210 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

does this not need the primary prefix? Cisco includes it.

(maybe it's already there, tho. to be investigated)

Comments from Reviewable

arifogel · 2017-12-19T19:18:31Z

Reviewed 35 of 35 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Interface.java, line 378 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Changed this to Sorted since it was initialized as a TreeSet and the refs broke otherwise.

Hmm somewhere we might need to remember order of secondary ips/prefixes, but this is fine for now.

projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/NetworkAcl.java, line 95 at r1 (raw file):

      }
    }
    List<IpAccessListLine> lines = new ArrayList<>(lineMap.values());

Can we use ImmutableList here?

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromDestinationPrefixList.java, line 45 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

could add a helper getter to RouteFilterList to take over these ~10 lines of code, if useful. Used in 4 or 5 FwFrom rules.

As long as it's well-tested, sure.

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromFragmentOffset.java, line 28 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

this was a set, not an add, so I kept it that way. Just noting the difference.

Actually most of the places you did concatenation would be fine as set, since there is no other place stuff could be added. I don't see it being a particularly important issue at the moment since there is no behavior change.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java, line 1210 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

(maybe it's already there, tho. to be investigated)

I think it was already there, unlike Cisco.

Comments from Reviewable

dhalperi · 2017-12-19T19:57:26Z

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java, line 1210 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I think it was already there, unlike Cisco.

Yes, it was already there. Found in enterIfi_address in ConfigurationBuilder.java

Comments from Reviewable

dhalperi · 2017-12-19T19:58:43Z

Review status: 28 of 38 files reviewed at latest revision, 4 unresolved discussions.

projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/NetworkAcl.java, line 95 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Can we use ImmutableList here?

Done.

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromDestinationPrefixList.java, line 45 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

As long as it's well-tested, sure.

Done.

projects/batfish/src/main/java/org/batfish/representation/juniper/FwFromFragmentOffset.java, line 28 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Actually most of the places you did concatenation would be fine as set, since there is no other place stuff could be added. I don't see it being a particularly important issue at the moment since there is no behavior change.

Ack.

Comments from Reviewable

arifogel · 2017-12-19T21:14:30Z

Reviewed 9 of 9 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 3265 at r2 (raw file):

    for (ExtendedAccessListLine fromLine : eaList.getLines()) {
      RouteFilterLine newLine = toRouteFilterLine(fromLine);
      newList.addLine(newLine);

I think this would be much more efficient without calls to addLine. That is, just build up the list and set the lines once. We know it's safe because the RouteFilterList is created here.

projects/batfish/src/main/java/org/batfish/representation/vyos/VyosConfiguration.java, line 324 at r2 (raw file):

      RouteFilterLine newLine =
          new RouteFilterLine(rule.getAction(), rule.getPrefix(), rule.getLengthRange());
      newList.addLine(newLine);

We should avoid using addLine here and just set the list. If this and my other mention are the only places that function is used, I think we should remove it.

Comments from Reviewable

dhalperi · 2017-12-19T21:24:31Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 3265 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I think this would be much more efficient without calls to addLine. That is, just build up the list and set the lines once. We know it's safe because the RouteFilterList is created here.

Done.

projects/batfish/src/main/java/org/batfish/representation/vyos/VyosConfiguration.java, line 324 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

We should avoid using addLine here and just set the list. If this and my other mention are the only places that function is used, I think we should remove it.

Done.

The function is used in 15 other places, not touched here.

Comments from Reviewable

arifogel · 2017-12-20T03:23:38Z

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

dhalperi assigned arifogel Dec 18, 2017

dhalperi force-pushed the immutable-data-model branch 2 times, most recently from ca70799 to 31c6c08 Compare December 19, 2017 06:02

dhalperi added 7 commits December 18, 2017 22:21

Data model: make a few key structures immutable

7959fc6

Most of the memory in the process is taken up by empty TreeSets and the like. If we use Immutable/Empty data structures for these, we'll save a huge amount of RAM.

IptableVendorConfiguration: build ACL lines first

8253138

CiscoConfiguration: changes to support immutable data model

fa952e3

IpTablesVendorConfiguration: support immutable datamodel in builder

cf78646

HostInterface; support immutable datamodel in builder

fa6fb6e

Aws: support immutable datamodel in builder

47299dc

Juniper: support immutable data model in builders

3af3d92

dhalperi force-pushed the immutable-data-model branch from 31c6c08 to 3af3d92 Compare December 19, 2017 06:35

SMT: changes for HeaderSpace with immutable data structures

c94cd1d

dhalperi changed the title ~~[draft] Steps towards an Immutable data model~~ HeaderSpace: use immutable structures internally Dec 19, 2017

dhalperi force-pushed the immutable-data-model branch from 7abcd16 to 6a63b85 Compare December 19, 2017 19:59

changes for review

ad819a7

dhalperi force-pushed the immutable-data-model branch from 6a63b85 to ad819a7 Compare December 19, 2017 20:11

routeFilterList.addLine cleanup

ae400d1

arifogel merged commit 4048ac9 into master Dec 20, 2017

arifogel deleted the immutable-data-model branch December 20, 2017 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HeaderSpace: use immutable structures internally #780

HeaderSpace: use immutable structures internally #780

dhalperi commented Dec 18, 2017 •

edited

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 19, 2017

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 20, 2017

HeaderSpace: use immutable structures internally #780

HeaderSpace: use immutable structures internally #780

Conversation

dhalperi commented Dec 18, 2017 • edited

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 19, 2017

dhalperi commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 19, 2017

dhalperi commented Dec 19, 2017

arifogel commented Dec 20, 2017

dhalperi commented Dec 18, 2017 •

edited