Optimize AbstractBestPathSelector

There are inefficiencies in how we compare local preference, let's
refactor the code to have clear comparisons without unnecessary
boxing.

Also remove implicit boxing of AS numbers, as we are already getting
them as primitive longs.

Change-Id: I3cbba14fa2c2bd2add35c0f827da8ba4a06a0b4b
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/api/BestPathState.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/api/BestPathState.java
index 4ece8c99a696188bb19a64bb9b7596ea3aa1a288..317bfefaa18b5dde53c762ed9afa39f8824c06f9 100644 (file)
--- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/api/BestPathState.java
+++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/api/BestPathState.java
@@ -17,8 +17,7 @@ public interface BestPathState {
     @Nullable
     Long getLocalPref();
 
-    @Nullable
-    Long getMultiExitDisc();
+    long getMultiExitDisc();
 
     @Nullable
     BgpOrigin getOrigin();

diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/BestPathStateImpl.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/BestPathStateImpl.java
index ad18c3218364cca4f202d82559b329113f3b947a..2ea80a49ff5038733407c4f2adb486b5bf1b509b 100644 (file)
--- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/BestPathStateImpl.java
+++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/BestPathStateImpl.java
@@ -28,7 +28,7 @@ public final class BestPathStateImpl implements BestPathState {
     private long peerAs = 0L;
     private int asPathLength = 0;
     private Long localPref;
-    private Long multiExitDisc;
+    private long multiExitDisc;
     private BgpOrigin origin;
     private boolean resolved;
 
@@ -64,7 +64,12 @@ public final class BestPathStateImpl implements BestPathState {
         localPref = attrLocalPref != null ? attrLocalPref.getPref() : null;
 
         final MultiExitDisc attrMed = attributes.getMultiExitDisc();
-        multiExitDisc = attrMed != null ? attrMed.getMed() : null;
+        if (attrMed != null) {
+            final Long med = attrMed.getMed();
+            multiExitDisc = med == null ? 0L : med;
+        } else {
+            multiExitDisc = 0L;
+        }
 
         final Origin attrOrigin = attributes.getOrigin();
         origin = attrOrigin != null ? attrOrigin.getValue() : null;
@@ -87,7 +92,7 @@ public final class BestPathStateImpl implements BestPathState {
     }
 
     @Override
-    public Long getMultiExitDisc() {
+    public long getMultiExitDisc() {
         resolveValues();
         return this.multiExitDisc;
     }
@@ -135,7 +140,7 @@ public final class BestPathStateImpl implements BestPathState {
         int result = 1;
         result = prime * result + this.attributes.hashCode();
         result = prime * result + (this.localPref == null ? 0 : this.localPref.hashCode());
-        result = prime * result + (this.multiExitDisc == null ? 0 : this.multiExitDisc.hashCode());
+        result = prime * result + Long.hashCode(multiExitDisc);
         result = prime * result + (this.origin == null ? 0 : this.origin.hashCode());
         return result;
     }
@@ -159,11 +164,7 @@ public final class BestPathStateImpl implements BestPathState {
         } else if (!this.localPref.equals(other.localPref)) {
             return false;
         }
-        if (this.multiExitDisc == null) {
-            if (other.multiExitDisc != null) {
-                return false;
-            }
-        } else if (!this.multiExitDisc.equals(other.multiExitDisc)) {
+        if (this.multiExitDisc != other.multiExitDisc) {
             return false;
         }
         if (this.origin != other.origin) {

diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractBestPathSelector.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractBestPathSelector.java
index 26740f33f191fa7bf57bd687a19b20e1f7d81f6d..c349165e10a9617cb3577a3e9651f42081cd5d8c 100644 (file)
--- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractBestPathSelector.java
+++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractBestPathSelector.java
@@ -55,18 +55,21 @@ public class AbstractBestPathSelector {
          * FIXME: for eBGP cases (when the LOCAL_PREF is missing), we should assign a policy-based preference
          *        before we ever get here.
          */
-        if (this.bestState.getLocalPref() == null && state.getLocalPref() != null) {
-            return true;
-        }
-        if (this.bestState.getLocalPref() != null && state.getLocalPref() == null) {
-            return false;
-        }
-        if (state.getLocalPref() != null && state.getLocalPref() > this.bestState.getLocalPref()) {
+        final Long bestLocal = this.bestState.getLocalPref();
+        final Long stateLocal = state.getLocalPref();
+        if (stateLocal != null) {
+            if (bestLocal == null) {
+                return true;
+            }
+
+            final Boolean bool = firstLower(stateLocal, bestLocal);
+            if (bool != null) {
+                return bool;
+            }
+        } else if (bestLocal != null) {
             return false;
         }
-        if (state.getLocalPref() != null && state.getLocalPref() < this.bestState.getLocalPref()) {
-            return true;
-        }
+
         // 3. prefer learned path
         // - we assume that all paths are learned
 
@@ -85,8 +88,8 @@ public class AbstractBestPathSelector {
             return no.ordinal() > bo.ordinal();
         }
         // FIXME: we should be able to cache the best AS
-        final Long bestAs = this.bestState.getPeerAs();
-        final Long newAs = state.getPeerAs();
+        final long bestAs = this.bestState.getPeerAs();
+        final long newAs = state.getPeerAs();
 
         /*
          * Checks 6 and 7 are mutually-exclusive, as MEDs are comparable
@@ -95,12 +98,11 @@ public class AbstractBestPathSelector {
          * relationship.
          *
          */
-        if (bestAs.equals(newAs)) {
+        if (bestAs == newAs) {
             // 6. prefer the path with the lowest multi-exit discriminator (MED)
-            if (this.bestState.getMultiExitDisc() != null || state.getMultiExitDisc() != null) {
-                final Long bmed = this.bestState.getMultiExitDisc();
-                final Long nmed = state.getMultiExitDisc();
-                return nmed >= bmed;
+            final Boolean cmp = firstLower(this.bestState.getMultiExitDisc(), state.getMultiExitDisc());
+            if (cmp != null) {
+                return cmp;
             }
         } else {
             /*
@@ -146,4 +148,9 @@ public class AbstractBestPathSelector {
          */
         return true;
     }
+
+    private static Boolean firstLower(final long first, final long second) {
+        return first < second ? Boolean.TRUE : first == second ? null : Boolean.FALSE;
+
+    }
 }