perf(core): [SDK Overhead reduction for JVM 2] Short-circuit combined scope breadcrumbs

CHANGELOG.md

@@ -2,12 +2,24 @@
 
 ## Unreleased
 
+### Behavioral Changes
+
+- Collections returned by scope (e.g. `getBreadcrumbs`, `getTags`, `getAttachments`) are shared state and should not be mutated. ([#5541](https://github.com/getsentry/sentry-java/pull/5541))
+  - Previously, when going through `CombinedScopeView`, we were returning a copy where mutations didn't show up in the underlying scopes.
+  - This has now changed in order to reduce SDK overhead.
+
 ### Features
 
 - Add experimental `SentrySQLiteDriver` to `sentry-android-sqlite` for instrumenting `androidx.sqlite.SQLiteDriver` ([#5563](https://github.com/getsentry/sentry-java/pull/5563))
   - To use it, pass `SQLiteDriver` to `SentrySQLiteDriver.create(...)`
   - Requires `androidx.sqlite:sqlite` (2.5.0+) on runtime classpath (typically provided by Room or SQLDelight)
 
+### Internal
+
+- Reduce writer buffer size from 8192 to 512 ([#5544](https://github.com/getsentry/sentry-java/pull/5544))
+- Remove redundant event map copies ([#5536](https://github.com/getsentry/sentry-java/pull/5536))
+- Optimize combined scope by adding an early return if only one scope has data ([#5541](https://github.com/getsentry/sentry-java/pull/5541))
+
 ## 8.44.0
 
 ### Features
@@ -36,11 +48,6 @@
 - Fix attachments being duplicated on native events that carry scope attachments ([#5548](https://github.com/getsentry/sentry-java/pull/5548))
 - Fix performance collector scheduling many tasks in a row ([#5524](https://github.com/getsentry/sentry-java/pull/5524))
 
-### Internal
-
-- Reduce writer buffer size from 8192 to 512 ([#5544](https://github.com/getsentry/sentry-java/pull/5544))
-- Remove redundant event map copies ([#5536](https://github.com/getsentry/sentry-java/pull/5536))
-
 ## 8.43.2
 
 ### Improvements

sentry/src/main/java/io/sentry/CombinedScopeView.java

@@ -171,10 +171,31 @@ public void setFingerprint(@NotNull List<String> fingerprint) {
 
   @Override
   public @NotNull Queue<Breadcrumb> getBreadcrumbs() {
+    final @NotNull Queue<Breadcrumb> globalBreadcrumbs = globalScope.getBreadcrumbs();
+    final @NotNull Queue<Breadcrumb> isolationBreadcrumbs = isolationScope.getBreadcrumbs();
+    final @NotNull Queue<Breadcrumb> currentBreadcrumbs = scope.getBreadcrumbs();
+
+    final boolean hasGlobalBreadcrumbs = !globalBreadcrumbs.isEmpty();
+    final boolean hasIsolationBreadcrumbs = !isolationBreadcrumbs.isEmpty();
+    final boolean hasCurrentBreadcrumbs = !currentBreadcrumbs.isEmpty();
+
+    if (!hasGlobalBreadcrumbs && !hasIsolationBreadcrumbs && !hasCurrentBreadcrumbs) {
+      return getDefaultScopeValue(globalBreadcrumbs, isolationBreadcrumbs, currentBreadcrumbs);
+    }
+    if (!hasIsolationBreadcrumbs && !hasCurrentBreadcrumbs) {
+      return globalBreadcrumbs;
+    }
+    if (!hasGlobalBreadcrumbs && !hasCurrentBreadcrumbs) {
+      return isolationBreadcrumbs;
+    }
+    if (!hasGlobalBreadcrumbs && !hasIsolationBreadcrumbs) {
+      return currentBreadcrumbs;
+    }
+
     final @NotNull List<Breadcrumb> allBreadcrumbs = new ArrayList<>();
-    allBreadcrumbs.addAll(globalScope.getBreadcrumbs());
-    allBreadcrumbs.addAll(isolationScope.getBreadcrumbs());
-    allBreadcrumbs.addAll(scope.getBreadcrumbs());
+    allBreadcrumbs.addAll(globalBreadcrumbs);
+    allBreadcrumbs.addAll(isolationBreadcrumbs);
+    allBreadcrumbs.addAll(currentBreadcrumbs);
     Collections.sort(allBreadcrumbs);
 
     final @NotNull Queue<Breadcrumb> breadcrumbs =
@@ -224,10 +245,31 @@ public void clear() {
 
   @Override
   public @NotNull Map<String, String> getTags() {
+    final @NotNull Map<String, String> globalTags = globalScope.getTags();
+    final @NotNull Map<String, String> isolationTags = isolationScope.getTags();
+    final @NotNull Map<String, String> currentTags = scope.getTags();
+
+    final boolean hasGlobalTags = !globalTags.isEmpty();
+    final boolean hasIsolationTags = !isolationTags.isEmpty();
+    final boolean hasCurrentTags = !currentTags.isEmpty();
+
+    if (!hasGlobalTags && !hasIsolationTags && !hasCurrentTags) {
+      return getDefaultScopeValue(globalTags, isolationTags, currentTags);
+    }
+    if (!hasIsolationTags && !hasCurrentTags) {
+      return globalTags;
+    }
+    if (!hasGlobalTags && !hasCurrentTags) {
+      return isolationTags;
+    }
+    if (!hasGlobalTags && !hasIsolationTags) {
+      return currentTags;
+    }
+
     final @NotNull Map<String, String> allTags = new ConcurrentHashMap<>();
-    allTags.putAll(globalScope.getTags());
-    allTags.putAll(isolationScope.getTags());
-    allTags.putAll(scope.getTags());
+    allTags.putAll(globalTags);
+    allTags.putAll(isolationTags);
+    allTags.putAll(currentTags);
     return allTags;
   }
 
@@ -243,10 +285,32 @@ public void removeTag(@Nullable String key) {
 
   @Override
   public @NotNull Map<String, SentryAttribute> getAttributes() {
+    final @NotNull Map<String, SentryAttribute> globalAttributes = globalScope.getAttributes();
+    final @NotNull Map<String, SentryAttribute> isolationAttributes =
+        isolationScope.getAttributes();
+    final @NotNull Map<String, SentryAttribute> currentAttributes = scope.getAttributes();
+
+    final boolean hasGlobalAttributes = !globalAttributes.isEmpty();
+    final boolean hasIsolationAttributes = !isolationAttributes.isEmpty();
+    final boolean hasCurrentAttributes = !currentAttributes.isEmpty();
+
+    if (!hasGlobalAttributes && !hasIsolationAttributes && !hasCurrentAttributes) {
+      return getDefaultScopeValue(globalAttributes, isolationAttributes, currentAttributes);
+    }
+    if (!hasIsolationAttributes && !hasCurrentAttributes) {
+      return globalAttributes;
+    }
+    if (!hasGlobalAttributes && !hasCurrentAttributes) {
+      return isolationAttributes;
+    }
+    if (!hasGlobalAttributes && !hasIsolationAttributes) {
+      return currentAttributes;
+    }
+
     final @NotNull Map<String, SentryAttribute> allAttributes = new ConcurrentHashMap<>();
-    allAttributes.putAll(globalScope.getAttributes());
-    allAttributes.putAll(isolationScope.getAttributes());
-    allAttributes.putAll(scope.getAttributes());
+    allAttributes.putAll(globalAttributes);
+    allAttributes.putAll(isolationAttributes);
+    allAttributes.putAll(currentAttributes);
     return allAttributes;
   }
 
@@ -272,11 +336,32 @@ public void removeAttribute(@Nullable String key) {
 
   @Override
   public @NotNull Map<String, Object> getExtras() {
-    final @NotNull Map<String, Object> allTags = new ConcurrentHashMap<>();
-    allTags.putAll(globalScope.getExtras());
-    allTags.putAll(isolationScope.getExtras());
-    allTags.putAll(scope.getExtras());
-    return allTags;
+    final @NotNull Map<String, Object> globalExtras = globalScope.getExtras();
+    final @NotNull Map<String, Object> isolationExtras = isolationScope.getExtras();
+    final @NotNull Map<String, Object> currentExtras = scope.getExtras();
+
+    final boolean hasGlobalExtras = !globalExtras.isEmpty();
+    final boolean hasIsolationExtras = !isolationExtras.isEmpty();
+    final boolean hasCurrentExtras = !currentExtras.isEmpty();
+
+    if (!hasGlobalExtras && !hasIsolationExtras && !hasCurrentExtras) {
+      return getDefaultScopeValue(globalExtras, isolationExtras, currentExtras);
+    }
+    if (!hasIsolationExtras && !hasCurrentExtras) {
+      return globalExtras;
+    }
+    if (!hasGlobalExtras && !hasCurrentExtras) {
+      return isolationExtras;
+    }
+    if (!hasGlobalExtras && !hasIsolationExtras) {
+      return currentExtras;
+    }
+
+    final @NotNull Map<String, Object> allExtras = new ConcurrentHashMap<>();
+    allExtras.putAll(globalExtras);
+    allExtras.putAll(isolationExtras);
+    allExtras.putAll(currentExtras);
+    return allExtras;
   }
 
   @Override
@@ -342,6 +427,23 @@ public void removeContexts(@Nullable String key) {
     return getSpecificScope(null);
   }
 
+  private <T> @NotNull T getDefaultScopeValue(
+      final @NotNull T globalValue,
+      final @NotNull T isolationValue,
+      final @NotNull T currentValue) {
+    switch (getOptions().getDefaultScopeType()) {
+      case CURRENT:
+        return currentValue;
+      case ISOLATION:
+        return isolationValue;
+      case GLOBAL:
+        return globalValue;
+      default:
+        // calm the compiler
+        return currentValue;
+    }
+  }
+
   IScope getSpecificScope(final @Nullable ScopeType scopeType) {
     if (scopeType != null) {
       switch (scopeType) {
@@ -373,10 +475,31 @@ IScope getSpecificScope(final @Nullable ScopeType scopeType) {
 
   @Override
   public @NotNull List<Attachment> getAttachments() {
+    final @NotNull List<Attachment> globalAttachments = globalScope.getAttachments();
+    final @NotNull List<Attachment> isolationAttachments = isolationScope.getAttachments();
+    final @NotNull List<Attachment> currentAttachments = scope.getAttachments();
+
+    final boolean hasGlobalAttachments = !globalAttachments.isEmpty();
+    final boolean hasIsolationAttachments = !isolationAttachments.isEmpty();
+    final boolean hasCurrentAttachments = !currentAttachments.isEmpty();
+
+    if (!hasGlobalAttachments && !hasIsolationAttachments && !hasCurrentAttachments) {
+      return getDefaultScopeValue(globalAttachments, isolationAttachments, currentAttachments);
+    }
+    if (!hasIsolationAttachments && !hasCurrentAttachments) {
+      return globalAttachments;
+    }
+    if (!hasGlobalAttachments && !hasCurrentAttachments) {
+      return isolationAttachments;
+    }
+    if (!hasGlobalAttachments && !hasIsolationAttachments) {
+      return currentAttachments;
+    }
+
     final @NotNull List<Attachment> allAttachments = new CopyOnWriteArrayList<>();
-    allAttachments.addAll(globalScope.getAttachments());
-    allAttachments.addAll(isolationScope.getAttachments());
-    allAttachments.addAll(scope.getAttachments());
+    allAttachments.addAll(globalAttachments);
+    allAttachments.addAll(isolationAttachments);
+    allAttachments.addAll(currentAttachments);
     return allAttachments;
   }
 

sentry/src/test/java/io/sentry/CombinedScopeViewTest.kt

@@ -11,6 +11,7 @@ import junit.framework.TestCase.assertTrue
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertNotNull
+import kotlin.test.assertNotSame
 import kotlin.test.assertNull
 import kotlin.test.assertSame
 import org.junit.Assert.assertNotEquals
@@ -72,6 +73,74 @@ class CombinedScopeViewTest {
     assertEquals("current 2", breadcrumbs.poll().message)
   }
 
+  @Test
+  fun `returns single non-empty breadcrumb queue directly`() {
+    var combined = fixture.getSut()
+    fixture.globalScope.addBreadcrumb(Breadcrumb.info("global"))
+    assertSame(fixture.globalScope.breadcrumbs, combined.breadcrumbs)
+
+    combined = fixture.getSut()
+    fixture.isolationScope.addBreadcrumb(Breadcrumb.info("isolation"))
+    assertSame(fixture.isolationScope.breadcrumbs, combined.breadcrumbs)
+
+    combined = fixture.getSut()
+    fixture.scope.addBreadcrumb(Breadcrumb.info("current"))
+    assertSame(fixture.scope.breadcrumbs, combined.breadcrumbs)
+  }
+
+  @Test
+  fun `returns default write scope breadcrumbs when all scopes are empty`() {
+    val combined = fixture.getSut(SentryOptions().also { it.defaultScopeType = ScopeType.CURRENT })
+
+    assertSame(fixture.scope.breadcrumbs, combined.breadcrumbs)
+  }
+
+  @Test
+  fun `returns merged breadcrumb copy when multiple scopes have breadcrumbs`() {
+    val combined = fixture.getSut()
+
+    fixture.globalScope.addBreadcrumb(Breadcrumb.info("global"))
+    fixture.isolationScope.addBreadcrumb(Breadcrumb.info("isolation"))
+
+    val breadcrumbs = combined.breadcrumbs
+
+    assertNotSame(fixture.globalScope.breadcrumbs, breadcrumbs)
+    assertNotSame(fixture.isolationScope.breadcrumbs, breadcrumbs)
+    assertEquals(2, breadcrumbs.size)
+  }
+
+  @Test
+  fun `returns single non-empty combined collections directly`() {
+    val globalScope = mock<IScope>()
+    val isolationScope = mock<IScope>()
+    val scope = mock<IScope>()
+    val combined = CombinedScopeView(globalScope, isolationScope, scope)
+
+    val tags = mapOf("tag" to "value")
+    whenever(globalScope.tags).thenReturn(emptyMap())
+    whenever(isolationScope.tags).thenReturn(emptyMap())
+    whenever(scope.tags).thenReturn(tags)
+    assertSame(tags, combined.tags)
+
+    val attributes = mapOf("attribute" to SentryAttribute.named("attribute", "value"))
+    whenever(globalScope.attributes).thenReturn(emptyMap())
+    whenever(isolationScope.attributes).thenReturn(emptyMap())
+    whenever(scope.attributes).thenReturn(attributes)
+    assertSame(attributes, combined.attributes)
+
+    val extras = mapOf<String, Any>("extra" to "value")
+    whenever(globalScope.extras).thenReturn(emptyMap())
+    whenever(isolationScope.extras).thenReturn(emptyMap())
+    whenever(scope.extras).thenReturn(extras)
+    assertSame(extras, combined.extras)
+
+    val attachments = listOf(createAttachment("attachment.png"))
+    whenever(globalScope.attachments).thenReturn(emptyList())
+    whenever(isolationScope.attachments).thenReturn(emptyList())
+    whenever(scope.attachments).thenReturn(attachments)
+    assertSame(attachments, combined.attachments)
+  }
+
   @Test
   fun `oldest breadcrumbs are dropped first`() {
     val options = SentryOptions().also { it.maxBreadcrumbs = 5 }