perf: trim stacktraces before constructing POJOs

bugsnag · Jun 21, 2021 · eb094e9 · eb094e9
1 parent d73e028
commit eb094e9
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 51 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,9 @@
 * Optimize metadata implementation by reducing type casts
   [#1277](https://github.com/bugsnag/bugsnag-android/pull/1277)
 
+* Trim stacktraces to <200 frames before attempting to construct POJOs
+  [#1281](https://github.com/bugsnag/bugsnag-android/pull/1281)
+
 ## 5.9.4 (2021-05-26)
 
 * Unity: add methods for setting autoNotify and autoDetectAnrs

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ErrorInternal.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ErrorInternal.kt
@@ -15,8 +15,7 @@ internal class ErrorInternal @JvmOverloads internal constructor(
                 .mapTo(mutableListOf()) { currentEx ->
                     // Somehow it's possible for stackTrace to be null in rare cases
                     val stacktrace = currentEx.stackTrace ?: arrayOf<StackTraceElement>()
-                    val trace =
-                        Stacktrace.stacktraceFromJavaTrace(stacktrace, projectPackages, logger)
+                    val trace = Stacktrace(stacktrace, projectPackages, logger)
                     val errorInternal =
                         ErrorInternal(currentEx.javaClass.name, currentEx.localizedMessage, trace)
 

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Stacktrace.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/Stacktrace.kt
@@ -27,38 +27,6 @@ internal class Stacktrace : JsonStream.Streamable {
             }
             return null
         }
-
-        fun stacktraceFromJavaTrace(
-            stacktrace: Array<StackTraceElement>,
-            projectPackages: Collection<String>,
-            logger: Logger
-        ): Stacktrace {
-            val frames = stacktrace.mapNotNull { serializeStackframe(it, projectPackages, logger) }
-            return Stacktrace(frames)
-        }
-
-        private fun serializeStackframe(
-            el: StackTraceElement,
-            projectPackages: Collection<String>,
-            logger: Logger
-        ): Stackframe? {
-            try {
-                val methodName = when {
-                    el.className.isNotEmpty() -> el.className + "." + el.methodName
-                    else -> el.methodName
-                }
-
-                return Stackframe(
-                    methodName,
-                    if (el.fileName == null) "Unknown" else el.fileName,
-                    el.lineNumber,
-                    inProject(el.className, projectPackages)
-                )
-            } catch (lineEx: Exception) {
-                logger.w("Failed to serialize stacktrace", lineEx)
-                return null
-            }
-        }
     }
 
     val trace: List<Stackframe>
@@ -67,13 +35,52 @@ internal class Stacktrace : JsonStream.Streamable {
         trace = limitTraceLength(frames)
     }
 
-    private fun <T> limitTraceLength(frames: List<T>): List<T> {
+    constructor(
+        stacktrace: Array<StackTraceElement>,
+        projectPackages: Collection<String>,
+        logger: Logger
+    ) {
+        val frames = limitTraceLength(stacktrace)
+        trace = frames.mapNotNull { serializeStackframe(it, projectPackages, logger) }
+    }
+
+    private fun limitTraceLength(frames: Array<StackTraceElement>): Array<StackTraceElement> {
+        return when {
+            frames.size >= STACKTRACE_TRIM_LENGTH -> frames.sliceArray(0 until STACKTRACE_TRIM_LENGTH)
+            else -> frames
+        }
+    }
+
+    private fun limitTraceLength(frames: List<Stackframe>): List<Stackframe> {
         return when {
             frames.size >= STACKTRACE_TRIM_LENGTH -> frames.subList(0, STACKTRACE_TRIM_LENGTH)
             else -> frames
         }
     }
 
+    private fun serializeStackframe(
+        el: StackTraceElement,
+        projectPackages: Collection<String>,
+        logger: Logger
+    ): Stackframe? {
+        try {
+            val methodName = when {
+                el.className.isNotEmpty() -> el.className + "." + el.methodName
+                else -> el.methodName
+            }
+
+            return Stackframe(
+                methodName,
+                if (el.fileName == null) "Unknown" else el.fileName,
+                el.lineNumber,
+                inProject(el.className, projectPackages)
+            )
+        } catch (lineEx: Exception) {
+            logger.w("Failed to serialize stacktrace", lineEx)
+            return null
+        }
+    }
+
     @Throws(IOException::class)
     override fun toStream(writer: JsonStream) {
         writer.beginArray()

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ThreadState.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ThreadState.kt
@@ -64,7 +64,7 @@ internal class ThreadState @Suppress("LongParameterList") @JvmOverloads construc
                 val trace = stackTraces[thread]
 
                 if (trace != null) {
-                    val stacktrace = Stacktrace.stacktraceFromJavaTrace(trace, projectPackages, logger)
+                    val stacktrace = Stacktrace(trace, projectPackages, logger)
                     val errorThread = thread.id == currentThreadId
                     Thread(thread.id, thread.name, ThreadType.ANDROID, errorThread, stacktrace, logger)
                 } else {

diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/EventSerializationTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/EventSerializationTest.kt
@@ -38,7 +38,7 @@ internal class EventSerializationTest {
 
                 // threads included
                 createEvent {
-                    val stacktrace = Stacktrace.stacktraceFromJavaTrace(arrayOf(), emptySet(), NoopLogger)
+                    val stacktrace = Stacktrace(arrayOf(), emptySet(), NoopLogger)
                     it.threads.clear()
                     it.threads.add(Thread(5, "main", ThreadType.ANDROID, true, stacktrace, NoopLogger))
                 },
@@ -53,7 +53,7 @@ internal class EventSerializationTest {
                     val crumb = Breadcrumb("hello world", BreadcrumbType.MANUAL, mutableMapOf(), Date(0), NoopLogger)
                     it.breadcrumbs = listOf(crumb)
 
-                    val stacktrace = Stacktrace.stacktraceFromJavaTrace(arrayOf(), emptySet(), NoopLogger)
+                    val stacktrace = Stacktrace(arrayOf(), emptySet(), NoopLogger)
                     val err = Error(ErrorInternal("WhoopsException", "Whoops", stacktrace), NoopLogger)
                     it.errors.clear()
                     it.errors.add(err)

diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/NullMetadataTest.java b/bugsnag-android-core/src/test/java/com/bugsnag/android/NullMetadataTest.java
@@ -45,7 +45,7 @@ public void testSecondErrorDefaultMetadata() {
                 = SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION);
         Event event = new Event(new RuntimeException(), config, reason, NoopLogger.INSTANCE);
         List<String> projectPackages = Collections.emptyList();
-        Stacktrace trace = Stacktrace.Companion.stacktraceFromJavaTrace(new StackTraceElement[]{},
+        Stacktrace trace = new Stacktrace(new StackTraceElement[]{},
                 projectPackages,
                 NoopLogger.INSTANCE);
         Error err = new Error(new ErrorInternal("RuntimeException", "Something broke",

diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/StacktraceSerializationTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/StacktraceSerializationTest.kt
@@ -19,7 +19,7 @@ internal class StacktraceSerializationTest {
                 "stacktrace",
 
                 // empty stacktrace element ctor
-                Stacktrace.stacktraceFromJavaTrace(arrayOf(), emptySet(), NoopLogger),
+                Stacktrace(arrayOf(), emptySet(), NoopLogger),
 
                 // empty custom frames ctor
                 Stacktrace(listOf(frame)),
@@ -37,13 +37,13 @@ internal class StacktraceSerializationTest {
         }
 
         private fun basic() =
-            Stacktrace.stacktraceFromJavaTrace(
+            Stacktrace(
                 RuntimeException("Whoops").stackTrace.sliceArray(IntRange(0, 1)),
                 emptySet(),
                 NoopLogger
             )
 
-        private fun inProject() = Stacktrace.stacktraceFromJavaTrace(
+        private fun inProject() = Stacktrace(
             RuntimeException("Whoops").stackTrace.sliceArray(IntRange(0, 1)),
             setOf("com.bugsnag.android"),
             NoopLogger
@@ -53,7 +53,7 @@ internal class StacktraceSerializationTest {
             val elements = (0..999).map {
                 StackTraceElement("SomeClass", "someMethod", "someFile", it)
             }
-            return Stacktrace.stacktraceFromJavaTrace(elements.toTypedArray(), emptyList(), NoopLogger)
+            return Stacktrace(elements.toTypedArray(), emptyList(), NoopLogger)
         }
 
         private fun trimStacktraceListCtor(): Stacktrace {

diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/StacktraceTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/StacktraceTest.kt
@@ -6,10 +6,9 @@ import org.junit.Test
 class StacktraceTest {
 
     @Test
-    fun stackframeLimits() {
-        val stackList = mutableListOf<Stackframe>()
-        for (i in 1..300) {
-            stackList.add(Stackframe("A", "B", i, true))
+    fun stackframeListTrimmed() {
+        val stackList = (1..300).map { index ->
+            Stackframe("A", "B", index, true)
         }
         val stacktrace = Stacktrace(stackList)
         // Confirm the length of the stackList
@@ -18,4 +17,18 @@ class StacktraceTest {
         assertEquals(1, stacktrace.trace.first().lineNumber)
         assertEquals(200, stacktrace.trace.last().lineNumber)
     }
+
+    @Test
+    fun stacktraceElementArrayTrimmed() {
+        val trace = (1..300).map { index ->
+            StackTraceElement("A", "B", "C", index)
+        }.toTypedArray()
+
+        val stacktrace = Stacktrace(trace, emptyList(), NoopLogger)
+        // Confirm the length of the stackList
+        assertEquals(300, trace.size)
+        assertEquals(200, stacktrace.trace.size)
+        assertEquals(1, stacktrace.trace.first().lineNumber)
+        assertEquals(200, stacktrace.trace.last().lineNumber)
+    }
 }
diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/ThreadSerializationTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/ThreadSerializationTest.kt
@@ -24,7 +24,7 @@ internal class ThreadSerializationTest {
                 "main-one",
                 ThreadType.ANDROID,
                 true,
-                Stacktrace.stacktraceFromJavaTrace(
+                Stacktrace(
                     stacktrace,
                     emptySet(),
                     NoopLogger
@@ -43,7 +43,7 @@ internal class ThreadSerializationTest {
                 "main-one",
                 ThreadType.ANDROID,
                 false,
-                Stacktrace.stacktraceFromJavaTrace(
+                Stacktrace(
                     stacktrace1,
                     emptySet(),
                     NoopLogger
@@ -66,7 +66,7 @@ internal class ThreadSerializationTest {
                     StackTraceElement("Runner", "runFunc", "Runner.java", 14),
                     StackTraceElement("App", "launch", "App.java", 70)
                 )
-            val trace = Stacktrace.stacktraceFromJavaTrace(
+            val trace = Stacktrace(
                 stacktrace,
                 emptyList(),
                 NoopLogger
@@ -88,7 +88,7 @@ internal class ThreadSerializationTest {
                     StackTraceElement("Runner", "runFunc", "Runner.java", 14),
                     StackTraceElement("App", "launch", "App.java", 70)
                 )
-            val trace = Stacktrace.stacktraceFromJavaTrace(
+            val trace = Stacktrace(
                 stacktrace,
                 emptyList(),
                 NoopLogger