diff --git a/CHANGELOG.md b/CHANGELOG.md index e3e26188c64..52ea22d9bda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ - Pass OpenTelemetry span attributes into TracesSampler callback ([#4253](https://github.com/getsentry/sentry-java/pull/4253)) - `SamplingContext` now has a `getAttribute` method that grants access to OpenTelemetry span attributes via their String key (e.g. `http.request.method`) - Fix AbstractMethodError when using SentryTraced for Jetpack Compose ([#4255](https://github.com/getsentry/sentry-java/pull/4255)) +- Avoid unnecessary copies when using `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247)) + - This affects in particular `SentryTracer.getLatestActiveSpan` which would have previously copied all child span references. This may have caused `OutOfMemoryError` on certain devices due to high frequency of calling the method. ### Features diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index d5b24621973..2c6aaaea2cf 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -6095,6 +6095,7 @@ public final class io/sentry/util/CollectionUtils { public static fun newArrayList (Ljava/util/List;)Ljava/util/List; public static fun newConcurrentHashMap (Ljava/util/Map;)Ljava/util/Map; public static fun newHashMap (Ljava/util/Map;)Ljava/util/Map; + public static fun reverseListIterator (Ljava/util/concurrent/CopyOnWriteArrayList;)Ljava/util/ListIterator; public static fun size (Ljava/lang/Iterable;)I } diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index cc832a136ef..af5459a1a37 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -5,9 +5,9 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.TransactionNameSource; import io.sentry.util.AutoClosableReentrantLock; +import io.sentry.util.CollectionUtils; import io.sentry.util.Objects; import io.sentry.util.SpanUtils; -import java.util.ArrayList; import java.util.List; import java.util.ListIterator; import java.util.Map; @@ -155,7 +155,9 @@ private void onDeadlineTimeoutReached() { // abort all child-spans first, this ensures the transaction can be finished, // even if waitForChildren is true // iterate in reverse order to ensure leaf spans are processed before their parents - @NotNull final ListIterator iterator = children.listIterator(children.size()); + @NotNull + final ListIterator iterator = + CollectionUtils.reverseListIterator((CopyOnWriteArrayList) this.children); while (iterator.hasPrevious()) { @NotNull final Span span = iterator.previous(); span.setSpanFinishedCallback(null); @@ -677,14 +679,13 @@ private void updateBaggageValues(final @NotNull Baggage baggage) { } private boolean hasAllChildrenFinished() { - final List spans = new ArrayList<>(this.children); - if (!spans.isEmpty()) { - for (final Span span : spans) { - // This is used in the spanFinishCallback, when the span isn't finished, but has a finish - // date - if (!span.isFinished() && span.getFinishDate() == null) { - return false; - } + @NotNull final ListIterator iterator = this.children.listIterator(); + while (iterator.hasNext()) { + @NotNull final Span span = iterator.next(); + // This is used in the spanFinishCallback, when the span isn't finished, but has a finish + // date + if (!span.isFinished() && span.getFinishDate() == null) { + return false; } } return true; @@ -909,12 +910,13 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac @Override public @Nullable ISpan getLatestActiveSpan() { - final List spans = new ArrayList<>(this.children); - if (!spans.isEmpty()) { - for (int i = spans.size() - 1; i >= 0; i--) { - if (!spans.get(i).isFinished()) { - return spans.get(i); - } + @NotNull + final ListIterator iterator = + CollectionUtils.reverseListIterator((CopyOnWriteArrayList) this.children); + while (iterator.hasPrevious()) { + @NotNull final Span span = iterator.previous(); + if (!span.isFinished()) { + return span; } } return null; diff --git a/sentry/src/main/java/io/sentry/util/CollectionUtils.java b/sentry/src/main/java/io/sentry/util/CollectionUtils.java index f3a0e1d9d74..266055fa1ce 100644 --- a/sentry/src/main/java/io/sentry/util/CollectionUtils.java +++ b/sentry/src/main/java/io/sentry/util/CollectionUtils.java @@ -4,8 +4,10 @@ import java.util.Collection; import java.util.HashMap; import java.util.List; +import java.util.ListIterator; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -179,4 +181,23 @@ public interface Predicate { public interface Mapper { R map(T t); } + + /** + * Returns a reverse iterator, where the first (resp. last) valid call to `prev` returns the last + * (resp. first) element that would be returned when iterating forwards. Note that this differs + * from the behavior of e.g. `org.apache.commons.collections4.iterators.ReverseListIterator`, + * where you need to iterate using `next` instead. We use the concrete type `CopyOnWriteArrayList` + * here as we are relying on the fact that its copy constructor only copies the reference to an + * internal array. We don't want to use this for other `List` implementations, as it could lead to + * an unnecessary copy of the elements instead. + * + * @param list the `CopyOnWriteArrayList` to get the reverse iterator for + * @param the type + * @return a reverse iterator over `list` + */ + public static @NotNull ListIterator reverseListIterator( + final @NotNull CopyOnWriteArrayList list) { + final @NotNull CopyOnWriteArrayList copy = new CopyOnWriteArrayList<>(list); + return copy.listIterator(copy.size()); + } } diff --git a/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt b/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt index ebdaff477b1..3772bb2aeb9 100644 --- a/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt +++ b/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt @@ -2,6 +2,7 @@ package io.sentry.util import io.sentry.JsonObjectReader import java.io.StringReader +import java.util.concurrent.CopyOnWriteArrayList import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -79,4 +80,26 @@ class CollectionUtilsTest { fun `contains returns false if element is not present`() { assertFalse(CollectionUtils.contains(arrayOf("one", "two", "three"), "four")) } + + @Test + fun `reverseListIterator returns empty iterator if list is empty`() { + val list = CopyOnWriteArrayList() + val iterator = CollectionUtils.reverseListIterator(list) + assertFalse(iterator.hasNext()) + assertFalse(iterator.hasPrevious()) + } + + @Test + fun `reverseListIterator returns reversed iterator if list is not empty`() { + val elements = listOf("one", "two", "three") + val list = CopyOnWriteArrayList(elements) + val iterator = CollectionUtils.reverseListIterator(list) + assertFalse(iterator.hasNext()) + assertTrue(iterator.hasPrevious()) + val reversedElements = mutableListOf() + while (iterator.hasPrevious()) { + reversedElements.add(iterator.previous()) + } + assertEquals(elements.reversed(), reversedElements) + } }