Committed by
Yuta Higuchi
Workaround for ConcurrentModificationException
- Workaround fix for ONOS-1193 - If locking the whole link collection damages the performance, apply the fix suggested on GossipLinkStore comment. Change-Id: Idd4d2e5b8e3a50843fe7abd10a615cfbd9a16478 (cherry picked from commit 4d973eba)
Showing
2 changed files
with
39 additions
and
20 deletions
... | @@ -238,13 +238,26 @@ public class GossipLinkStore | ... | @@ -238,13 +238,26 @@ public class GossipLinkStore |
238 | @Override | 238 | @Override |
239 | public Set<Link> getEgressLinks(ConnectPoint src) { | 239 | public Set<Link> getEgressLinks(ConnectPoint src) { |
240 | Set<Link> egress = new HashSet<>(); | 240 | Set<Link> egress = new HashSet<>(); |
241 | - for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | 241 | + // |
242 | - if (linkKey.src().equals(src)) { | 242 | + // Change `srcLinks` to ConcurrentMap<DeviceId, (Concurrent)Set> |
243 | - Link link = links.get(linkKey); | 243 | + // to remove this synchronized block, if we hit performance issue. |
244 | - if (link != null) { | 244 | + // SetMultiMap#get returns wrapped collection to provide modifiable-view. |
245 | - egress.add(link); | 245 | + // And the wrapped collection is not concurrent access safe. |
246 | - } else { | 246 | + // |
247 | - log.debug("Egress link for {} was null, skipped", linkKey); | 247 | + // Our use case here does not require returned collection to be modifiable, |
248 | + // so the wrapped collection forces us to lock the whole multiset, | ||
249 | + // for benefit we don't need. | ||
250 | + // | ||
251 | + // Same applies to `dstLinks` | ||
252 | + synchronized (srcLinks) { | ||
253 | + for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | ||
254 | + if (linkKey.src().equals(src)) { | ||
255 | + Link link = links.get(linkKey); | ||
256 | + if (link != null) { | ||
257 | + egress.add(link); | ||
258 | + } else { | ||
259 | + log.debug("Egress link for {} was null, skipped", linkKey); | ||
260 | + } | ||
248 | } | 261 | } |
249 | } | 262 | } |
250 | } | 263 | } |
... | @@ -254,13 +267,15 @@ public class GossipLinkStore | ... | @@ -254,13 +267,15 @@ public class GossipLinkStore |
254 | @Override | 267 | @Override |
255 | public Set<Link> getIngressLinks(ConnectPoint dst) { | 268 | public Set<Link> getIngressLinks(ConnectPoint dst) { |
256 | Set<Link> ingress = new HashSet<>(); | 269 | Set<Link> ingress = new HashSet<>(); |
257 | - for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { | 270 | + synchronized (dstLinks) { |
258 | - if (linkKey.dst().equals(dst)) { | 271 | + for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { |
259 | - Link link = links.get(linkKey); | 272 | + if (linkKey.dst().equals(dst)) { |
260 | - if (link != null) { | 273 | + Link link = links.get(linkKey); |
261 | - ingress.add(link); | 274 | + if (link != null) { |
262 | - } else { | 275 | + ingress.add(link); |
263 | - log.debug("Ingress link for {} was null, skipped", linkKey); | 276 | + } else { |
277 | + log.debug("Ingress link for {} was null, skipped", linkKey); | ||
278 | + } | ||
264 | } | 279 | } |
265 | } | 280 | } |
266 | } | 281 | } | ... | ... |
... | @@ -146,9 +146,11 @@ public class SimpleLinkStore | ... | @@ -146,9 +146,11 @@ public class SimpleLinkStore |
146 | @Override | 146 | @Override |
147 | public Set<Link> getEgressLinks(ConnectPoint src) { | 147 | public Set<Link> getEgressLinks(ConnectPoint src) { |
148 | Set<Link> egress = new HashSet<>(); | 148 | Set<Link> egress = new HashSet<>(); |
149 | - for (LinkKey linkKey : srcLinks.get(src.deviceId())) { | 149 | + synchronized (srcLinks) { |
150 | - if (linkKey.src().equals(src)) { | 150 | + for (LinkKey linkKey : srcLinks.get(src.deviceId())) { |
151 | - egress.add(links.get(linkKey)); | 151 | + if (linkKey.src().equals(src)) { |
152 | + egress.add(links.get(linkKey)); | ||
153 | + } | ||
152 | } | 154 | } |
153 | } | 155 | } |
154 | return egress; | 156 | return egress; |
... | @@ -157,9 +159,11 @@ public class SimpleLinkStore | ... | @@ -157,9 +159,11 @@ public class SimpleLinkStore |
157 | @Override | 159 | @Override |
158 | public Set<Link> getIngressLinks(ConnectPoint dst) { | 160 | public Set<Link> getIngressLinks(ConnectPoint dst) { |
159 | Set<Link> ingress = new HashSet<>(); | 161 | Set<Link> ingress = new HashSet<>(); |
160 | - for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { | 162 | + synchronized (dstLinks) { |
161 | - if (linkKey.dst().equals(dst)) { | 163 | + for (LinkKey linkKey : dstLinks.get(dst.deviceId())) { |
162 | - ingress.add(links.get(linkKey)); | 164 | + if (linkKey.dst().equals(dst)) { |
165 | + ingress.add(links.get(linkKey)); | ||
166 | + } | ||
163 | } | 167 | } |
164 | } | 168 | } |
165 | return ingress; | 169 | return ingress; | ... | ... |
-
Please register or login to post a comment