From 9859aa5297b339f76719796a2f495823deccdc82 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 17:27:29 +0100 Subject: [PATCH 1/9] Avoid copying and iterate correctly on `SentryTracer.children` --- .../src/main/java/io/sentry/SentryTracer.java | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 36329db7a71..52a50182833 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -7,7 +7,6 @@ import io.sentry.util.AutoClosableReentrantLock; 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 +154,10 @@ 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 = this.children.listIterator(); + while (iterator.hasNext()) { + iterator.next(); + } 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.previous(); + // 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,14 @@ 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 = this.children.listIterator(); + while (iterator.hasNext()) { + iterator.next(); + } + while (iterator.hasPrevious()) { + @NotNull final Span span = iterator.previous(); + if (!span.isFinished()) { + return span; } } return null; From 4ef26722d4683257147119ef6574f47f411da64d Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 18:01:12 +0100 Subject: [PATCH 2/9] another place --- sentry/src/main/java/io/sentry/Scope.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index 4dacc8e4dac..d8eecb4708d 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -759,7 +759,7 @@ public void removeContexts(final @NotNull String key) { @NotNull @Override public List getAttachments() { - return new CopyOnWriteArrayList<>(attachments); + return attachments; } /** From 0ad1efb7582780d0efb29275faf7b842a090aec2 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 18:08:43 +0100 Subject: [PATCH 3/9] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 440881b5940..1dc5db2f5b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Fixes - Add support for setting in-app-includes/in-app-excludes via AndroidManifest.xml ([#4240](https://github.com/getsentry/sentry-java/pull/4240)) +- Avoid unnecessary copies and ensure thread-safety when using `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247)) + - This affects in particular `SentryTracer.getLatestActiveSpan` which would have previously unnecessarily copied all children spans, with high impact on memory usage ### Features From a0872d0b6e3ad6d65fddeb2d24cd5c8fa6597a10 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 22:06:18 +0100 Subject: [PATCH 4/9] fix --- sentry/src/main/java/io/sentry/SentryTracer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 52a50182833..7e9de25fbfe 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -681,7 +681,7 @@ private void updateBaggageValues(final @NotNull Baggage baggage) { private boolean hasAllChildrenFinished() { @NotNull final ListIterator iterator = this.children.listIterator(); while (iterator.hasNext()) { - @NotNull final Span span = iterator.previous(); + @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) { From 6223546ed89735bfbc0b24eba9536c40b0861f1b Mon Sep 17 00:00:00 2001 From: lcian Date: Tue, 11 Mar 2025 09:09:40 +0100 Subject: [PATCH 5/9] remove unnecessary test --- sentry/src/test/java/io/sentry/ScopeTest.kt | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/sentry/src/test/java/io/sentry/ScopeTest.kt b/sentry/src/test/java/io/sentry/ScopeTest.kt index b8025735e8a..b4b46f45050 100644 --- a/sentry/src/test/java/io/sentry/ScopeTest.kt +++ b/sentry/src/test/java/io/sentry/ScopeTest.kt @@ -906,18 +906,6 @@ class ScopeTest { assertTrue(cloned.attachments is CopyOnWriteArrayList) } - @Test - fun `getAttachments returns new instance`() { - val scope = Scope(SentryOptions()) - scope.addAttachment(Attachment("")) - - assertNotSame( - scope.attachments, - scope.attachments, - "Scope.attachments must return a new instance on each call." - ) - } - @Test fun `clearAttachments clears all attachments`() { val scope = Scope(SentryOptions()) From 05bfb838b7ff2ced8fa73ea7a65e17c25a9870bc Mon Sep 17 00:00:00 2001 From: lcian Date: Thu, 13 Mar 2025 11:25:06 +0100 Subject: [PATCH 6/9] wip --- CHANGELOG.md | 2 +- sentry/src/main/java/io/sentry/Scope.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc5db2f5b7..6ba3cf22bd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixes - Add support for setting in-app-includes/in-app-excludes via AndroidManifest.xml ([#4240](https://github.com/getsentry/sentry-java/pull/4240)) -- Avoid unnecessary copies and ensure thread-safety when using `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247)) +- 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 unnecessarily copied all children spans, with high impact on memory usage ### Features diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index d8eecb4708d..4dacc8e4dac 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -759,7 +759,7 @@ public void removeContexts(final @NotNull String key) { @NotNull @Override public List getAttachments() { - return attachments; + return new CopyOnWriteArrayList<>(attachments); } /** From 04c2e168764e908b46a5d0be044a054889036c62 Mon Sep 17 00:00:00 2001 From: lcian Date: Thu, 13 Mar 2025 12:14:45 +0100 Subject: [PATCH 7/9] improve --- sentry/api/sentry.api | 1 + .../src/main/java/io/sentry/SentryTracer.java | 16 ++++++------- .../java/io/sentry/util/CollectionUtils.java | 21 +++++++++++++++++ sentry/src/test/java/io/sentry/ScopeTest.kt | 12 ++++++++++ .../io/sentry/util/CollectionUtilsTest.kt | 23 +++++++++++++++++++ 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 1c4883e078e..6d68bd55909 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -6087,6 +6087,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 7e9de25fbfe..2303943b361 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -1,5 +1,7 @@ package io.sentry; +import static io.sentry.util.CollectionUtils.reverseListIterator; + import io.sentry.protocol.Contexts; import io.sentry.protocol.SentryId; import io.sentry.protocol.SentryTransaction; @@ -154,10 +156,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 = this.children.listIterator(); - while (iterator.hasNext()) { - iterator.next(); - } + @NotNull + final ListIterator iterator = + reverseListIterator((CopyOnWriteArrayList) this.children); while (iterator.hasPrevious()) { @NotNull final Span span = iterator.previous(); span.setSpanFinishedCallback(null); @@ -910,10 +911,9 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac @Override public @Nullable ISpan getLatestActiveSpan() { - @NotNull final ListIterator iterator = this.children.listIterator(); - while (iterator.hasNext()) { - iterator.next(); - } + @NotNull + final ListIterator iterator = + reverseListIterator((CopyOnWriteArrayList) this.children); while (iterator.hasPrevious()) { @NotNull final Span span = iterator.previous(); if (!span.isFinished()) { 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/ScopeTest.kt b/sentry/src/test/java/io/sentry/ScopeTest.kt index b4b46f45050..b8025735e8a 100644 --- a/sentry/src/test/java/io/sentry/ScopeTest.kt +++ b/sentry/src/test/java/io/sentry/ScopeTest.kt @@ -906,6 +906,18 @@ class ScopeTest { assertTrue(cloned.attachments is CopyOnWriteArrayList) } + @Test + fun `getAttachments returns new instance`() { + val scope = Scope(SentryOptions()) + scope.addAttachment(Attachment("")) + + assertNotSame( + scope.attachments, + scope.attachments, + "Scope.attachments must return a new instance on each call." + ) + } + @Test fun `clearAttachments clears all attachments`() { val scope = Scope(SentryOptions()) 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) + } } From 71e9529efef71861b28d5e4ee539158d8fbe7782 Mon Sep 17 00:00:00 2001 From: lcian Date: Thu, 13 Mar 2025 13:59:16 +0100 Subject: [PATCH 8/9] improve --- sentry/src/main/java/io/sentry/SentryTracer.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 2303943b361..5ee084e42fb 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -1,12 +1,11 @@ package io.sentry; -import static io.sentry.util.CollectionUtils.reverseListIterator; - import io.sentry.protocol.Contexts; import io.sentry.protocol.SentryId; 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.List; @@ -158,7 +157,7 @@ private void onDeadlineTimeoutReached() { // iterate in reverse order to ensure leaf spans are processed before their parents @NotNull final ListIterator iterator = - reverseListIterator((CopyOnWriteArrayList) this.children); + CollectionUtils.reverseListIterator((CopyOnWriteArrayList) this.children); while (iterator.hasPrevious()) { @NotNull final Span span = iterator.previous(); span.setSpanFinishedCallback(null); @@ -913,7 +912,7 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac public @Nullable ISpan getLatestActiveSpan() { @NotNull final ListIterator iterator = - reverseListIterator((CopyOnWriteArrayList) this.children); + CollectionUtils.reverseListIterator((CopyOnWriteArrayList) this.children); while (iterator.hasPrevious()) { @NotNull final Span span = iterator.previous(); if (!span.isFinished()) { From f56abbe959528ccd20f183c7a077ca8216be4a95 Mon Sep 17 00:00:00 2001 From: Lorenzo Cian Date: Fri, 14 Mar 2025 08:48:08 +0100 Subject: [PATCH 9/9] Update CHANGELOG.md Co-authored-by: Alexander Dinauer --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2d07f3ac88..52ea22d9bda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ - `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 unnecessarily copied all children spans, with high impact on memory usage + - 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