From 3ada5053cf1638e8d8ac554d0dd1bd7fc87223c8 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sat, 12 Oct 2019 13:45:58 +0200 Subject: [PATCH] Improve DpnTepStateCache performance Instead of forcing allocation of Strings all the time, reorganize the cache to have an explicit key. Also mark a FIXME to where we are forcing String/Uint64 conversions. Change-Id: I1ed8fbecd5dedf01b69b46ca7809b659eeb00b4d Signed-off-by: Robert Varga --- .../genius/itm/cache/CacheKey.java | 44 +++++++++++ .../genius/itm/cache/DpnTepStateCache.java | 79 +++++++++---------- 2 files changed, 83 insertions(+), 40 deletions(-) create mode 100644 itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/CacheKey.java diff --git a/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/CacheKey.java b/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/CacheKey.java new file mode 100644 index 000000000..0a5594805 --- /dev/null +++ b/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/CacheKey.java @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2019 PANTHEON.tech, s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.genius.itm.cache; + +import java.util.Objects; +import org.opendaylight.yangtools.yang.common.Uint64; + +final class CacheKey { + private final Uint64 src; + private final Uint64 dst; + + CacheKey(Uint64 src, Uint64 dst) { + // FIXME: are nulls allowed here? + this.src = src; + this.dst = dst; + } + + @Override + public int hashCode() { + return Objects.hashCode(src) * 31 + Objects.hashCode(dst); + } + + @Override + public boolean equals(Object obj) { + if (obj == this) { + return true; + } + if (!(obj instanceof CacheKey)) { + return false; + } + final CacheKey other = (CacheKey) obj; + return Objects.equals(src, other.src) && Objects.equals(dst, other.dst); + } + + @Override + public String toString() { + return src + ":" + dst; + } +} diff --git a/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/DpnTepStateCache.java b/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/DpnTepStateCache.java index b91d17a79..21dddcf81 100644 --- a/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/DpnTepStateCache.java +++ b/itm/itm-impl/src/main/java/org/opendaylight/genius/itm/cache/DpnTepStateCache.java @@ -53,10 +53,11 @@ import org.slf4j.LoggerFactory; @Singleton public class DpnTepStateCache extends DataObjectCache { - private static final Logger LOG = LoggerFactory.getLogger(DpnTepStateCache.class); private static final Logger EVENT_LOGGER = LoggerFactory.getLogger("GeniusEventLogger"); + private final ConcurrentMap dpnTepInterfaceMap = new ConcurrentHashMap<>(); + private final ConcurrentMap tunnelEndpointMap = new ConcurrentHashMap<>(); private final DataBroker dataBroker; private final JobCoordinator coordinator; private final DirectTunnelUtils directTunnelUtils; @@ -64,8 +65,6 @@ public class DpnTepStateCache extends DataObjectCache { private final UnprocessedNodeConnectorCache unprocessedNCCache; private final UnprocessedNodeConnectorEndPointCache unprocessedNodeConnectorEndPointCache; private final ManagedNewTransactionRunner txRunner; - private final ConcurrentMap dpnTepInterfaceMap = new ConcurrentHashMap<>(); - private final ConcurrentMap tunnelEndpointMap = new ConcurrentHashMap<>(); @Inject public DpnTepStateCache(DataBroker dataBroker, JobCoordinator coordinator, @@ -91,26 +90,29 @@ public class DpnTepStateCache extends DataObjectCache { protected void added(InstanceIdentifier path, DpnsTeps dpnsTeps) { String srcOfTunnel = dpnsTeps.getOfTunnel(); for (RemoteDpns remoteDpns : dpnsTeps.nonnullRemoteDpns().values()) { - final String dpn = getDpnId(dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); DpnTepInterfaceInfo value = new DpnTepInterfaceInfoBuilder() .setTunnelName(remoteDpns.getTunnelName()) .setIsMonitoringEnabled(remoteDpns.isMonitoringEnabled()) .setIsInternal(remoteDpns.isInternal()) .setTunnelType(dpnsTeps.getTunnelType()) .setRemoteDPN(remoteDpns.getDestinationDpnId()).build(); - dpnTepInterfaceMap.put(dpn, value); + final CacheKey key = new CacheKey(dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); + dpnTepInterfaceMap.put(key, value); addTunnelEndPointInfoToCache(remoteDpns.getTunnelName(), dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); //Process the unprocessed NodeConnector for the Tunnel, if present in the UnprocessedNodeConnectorCache + final String dpn = key.toString(); TunnelStateInfo tunnelStateInfoNew = null; - TunnelStateInfo tunnelStateInfo; try (Acquired lock = directTunnelUtils.lockTunnel(remoteDpns.getTunnelName())) { - if (srcOfTunnel != null && unprocessedNCCache.get(dpn) != null) { + if (srcOfTunnel != null) { tunnelStateInfo = unprocessedNCCache.remove(dpn); + if (tunnelStateInfo == null) { + tunnelStateInfo = unprocessedNCCache.remove(remoteDpns.getTunnelName()); + } } else { tunnelStateInfo = unprocessedNCCache.remove(remoteDpns.getTunnelName()); } @@ -122,7 +124,8 @@ public class DpnTepStateCache extends DataObjectCache { TunnelEndPointInfo tunnelEndPtInfo = getTunnelEndPointInfo(dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); TunnelStateInfoBuilder builder = new TunnelStateInfoBuilder() - .setNodeConnectorInfo(tunnelStateInfo.getNodeConnectorInfo()).setDpnTepInterfaceInfo(value) + .setNodeConnectorInfo(tunnelStateInfo.getNodeConnectorInfo()) + .setDpnTepInterfaceInfo(value) .setTunnelEndPointInfo(tunnelEndPtInfo); dpnTepsInfoCache.getDPNTepFromDPNId(dpnsTeps.getSourceDpnId()).ifPresent(builder::setSrcDpnTepsInfo); @@ -163,39 +166,40 @@ public class DpnTepStateCache extends DataObjectCache { @Override protected void removed(InstanceIdentifier path, DpnsTeps dpnsTeps) { for (RemoteDpns remoteDpns : dpnsTeps.nonnullRemoteDpns().values()) { - String fwkey = getDpnId(dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); - dpnTepInterfaceMap.remove(fwkey); + dpnTepInterfaceMap.remove(new CacheKey(dpnsTeps.getSourceDpnId(), remoteDpns.getDestinationDpnId())); tunnelEndpointMap.remove(remoteDpns.getTunnelName()); - String revkey = getDpnId(remoteDpns.getDestinationDpnId(), dpnsTeps.getSourceDpnId()); - dpnTepInterfaceMap.remove(revkey); + dpnTepInterfaceMap.remove(new CacheKey(remoteDpns.getDestinationDpnId(), dpnsTeps.getSourceDpnId())); } } public DpnTepInterfaceInfo getDpnTepInterface(Uint64 srcDpnId, Uint64 dstDpnId) { - DpnTepInterfaceInfo dpnTepInterfaceInfo = dpnTepInterfaceMap.get(getDpnId(srcDpnId, dstDpnId)); - if (dpnTepInterfaceInfo == null) { - try { - Optional dpnsTeps = super.get(srcDpnId); - if (dpnsTeps.isPresent()) { - DpnsTeps teps = dpnsTeps.get(); - teps.nonnullRemoteDpns().values().forEach(remoteDpns -> { - DpnTepInterfaceInfo value = new DpnTepInterfaceInfoBuilder() - .setTunnelName(remoteDpns.getTunnelName()) - .setIsMonitoringEnabled(remoteDpns.isMonitoringEnabled()) - .setIsInternal(remoteDpns.isInternal()) - .setTunnelType(teps.getTunnelType()) - .setRemoteDPN(remoteDpns.getDestinationDpnId()).build(); - dpnTepInterfaceMap.putIfAbsent(getDpnId(srcDpnId, remoteDpns.getDestinationDpnId()), value); - addTunnelEndPointInfoToCache(remoteDpns.getTunnelName(), - teps.getSourceDpnId(), remoteDpns.getDestinationDpnId()); - } - ); - } - } catch (ReadFailedException e) { - LOG.error("cache read for dpnID {} in DpnTepStateCache failed ", srcDpnId, e); + + CacheKey srcDst = new CacheKey(srcDpnId, dstDpnId); + DpnTepInterfaceInfo dpnTepInterfaceInfo = dpnTepInterfaceMap.get(srcDst); + if (dpnTepInterfaceInfo != null) { + return dpnTepInterfaceInfo; + } + + try { + Optional dpnsTeps = super.get(srcDpnId); + if (dpnsTeps.isPresent()) { + DpnsTeps teps = dpnsTeps.get(); + teps.nonnullRemoteDpns().values().forEach(remoteDpns -> { + DpnTepInterfaceInfo value = new DpnTepInterfaceInfoBuilder() + .setTunnelName(remoteDpns.getTunnelName()) + .setIsMonitoringEnabled(remoteDpns.isMonitoringEnabled()) + .setIsInternal(remoteDpns.isInternal()) + .setTunnelType(teps.getTunnelType()) + .setRemoteDPN(remoteDpns.getDestinationDpnId()).build(); + dpnTepInterfaceMap.putIfAbsent(new CacheKey(srcDpnId, remoteDpns.getDestinationDpnId()), value); + addTunnelEndPointInfoToCache(remoteDpns.getTunnelName(), teps.getSourceDpnId(), + remoteDpns.getDestinationDpnId()); + }); } + } catch (ReadFailedException e) { + LOG.error("cache read for dpnID {} in DpnTepStateCache failed ", srcDpnId, e); } - return dpnTepInterfaceMap.get(getDpnId(srcDpnId, dstDpnId)); + return dpnTepInterfaceMap.get(srcDst); } public void removeTepFromDpnTepInterfaceConfigDS(Uint64 srcDpnId) throws TransactionCommitFailedException { @@ -261,11 +265,6 @@ public class DpnTepStateCache extends DataObjectCache { return getDpnTepInterface(endPointInfo.getSrcEndPointInfo(), endPointInfo.getDstEndPointInfo()); } - // FIXME: this seems to be a cache key -- it should use a composite structure rather than string concat - private String getDpnId(Uint64 src, Uint64 dst) { - return src + ":" + dst; - } - public Interface getInterfaceFromCache(String tunnelName) { TunnelEndPointInfo endPointInfo = getTunnelEndPointInfoFromCache(tunnelName); Uint64 srcDpnId = endPointInfo.getSrcEndPointInfo(); @@ -291,7 +290,7 @@ public class DpnTepStateCache extends DataObjectCache { //Start: TunnelEndPoint Cache accessors private void addTunnelEndPointInfoToCache(String tunnelName, Uint64 srcEndPtInfo, Uint64 dstEndPtInfo) { - tunnelEndpointMap.put(tunnelName, getTunnelEndPointInfo(srcEndPtInfo,dstEndPtInfo)); + tunnelEndpointMap.put(tunnelName, getTunnelEndPointInfo(srcEndPtInfo, dstEndPtInfo)); } private static TunnelEndPointInfo getTunnelEndPointInfo(Uint64 srcEndPtInfo, Uint64 dstEndPtInfo) { -- 2.36.6