From a79001aab495ecc03e5ae576c0456e2b07d54138 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Wed, 4 Jan 2023 15:54:10 -0500 Subject: [PATCH] [Sb refactor] Add a reusable method to TableLogBufferFactory The majority of uses of LogBuffer and TableLogBuffer will be set up in the dagger graph, as the controllers logging to it are typically known at compile time. In the case of mobile, we are lazily creating the log buffers, and want to reference them by name. This CL adds a cache to TableLogBufferFactory, which is opt-in to avoid wasting memory caching every single log buffer. They are all already retained by DumpManager which throws an exception if a reregister is caught. Test: MobileConnectionsRepositoryTest Fixes: 264257095 Change-Id: I0e93e4317eb8108288983ffa01ccbc092e1e242e --- .../log/table/TableLogBufferFactory.kt | 31 +++++++++ .../demo/DemoMobileConnectionsRepository.kt | 2 +- .../prod/MobileConnectionRepositoryImpl.kt | 2 +- .../log/table/TableLogBufferFactoryTest.kt | 64 +++++++++++++++++++ .../prod/MobileConnectionsRepositoryTest.kt | 48 +++++++++++++- 5 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferFactoryTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt index 7a90a7470cd25..7ccc43ce62c27 100644 --- a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt +++ b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBufferFactory.kt @@ -29,6 +29,18 @@ constructor( private val dumpManager: DumpManager, private val systemClock: SystemClock, ) { + private val existingBuffers = mutableMapOf() + + /** + * Creates a new [TableLogBuffer]. This method should only be called from static contexts, where + * it is guaranteed only to be created one time. See [getOrCreate] for a cache-aware method of + * obtaining a buffer. + * + * @param name a unique table name + * @param maxSize the buffer max size. See [adjustMaxSize] + * + * @return a new [TableLogBuffer] registered with [DumpManager] + */ fun create( name: String, maxSize: Int, @@ -37,4 +49,23 @@ constructor( dumpManager.registerNormalDumpable(name, tableBuffer) return tableBuffer } + + /** + * Log buffers are retained indefinitely by [DumpManager], so that they can be represented in + * bugreports. Because of this, many of them are created statically in the Dagger graph. + * + * In the case where you have to create a logbuffer with a name only known at runtime, this + * method can be used to lazily create a table log buffer which is then cached for reuse. + * + * @return a [TableLogBuffer] suitable for reuse + */ + fun getOrCreate( + name: String, + maxSize: Int, + ): TableLogBuffer = + existingBuffers.getOrElse(name) { + val buffer = create(name, maxSize) + existingBuffers[name] = buffer + buffer + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt index 0e164e7ee859d..8ac12379e59e5 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt @@ -149,7 +149,7 @@ constructor( } private fun createDemoMobileConnectionRepo(subId: Int): DemoMobileConnectionRepository { - val tableLogBuffer = logFactory.create("DemoMobileConnectionLog [$subId]", 100) + val tableLogBuffer = logFactory.getOrCreate("DemoMobileConnectionLog [$subId]", 100) return DemoMobileConnectionRepository( subId, diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt index 0fa0fea0bebfa..4e42f9b31e5c0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt @@ -308,7 +308,7 @@ class MobileConnectionRepositoryImpl( networkNameSeparator: String, globalMobileDataSettingChangedEvent: Flow, ): MobileConnectionRepository { - val mobileLogger = logFactory.create(tableBufferLogName(subId), 100) + val mobileLogger = logFactory.getOrCreate(tableBufferLogName(subId), 100) return MobileConnectionRepositoryImpl( context, diff --git a/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferFactoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferFactoryTest.kt new file mode 100644 index 0000000000000..411b1bd04c526 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableLogBufferFactoryTest.kt @@ -0,0 +1,64 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.log.table + +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.dump.DumpManager +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.time.FakeSystemClock +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +@SmallTest +class TableLogBufferFactoryTest : SysuiTestCase() { + private val dumpManager: DumpManager = mock() + private val systemClock = FakeSystemClock() + private val underTest = TableLogBufferFactory(dumpManager, systemClock) + + @Test + fun `create - always creates new instance`() { + val b1 = underTest.create(NAME_1, SIZE) + val b1_copy = underTest.create(NAME_1, SIZE) + val b2 = underTest.create(NAME_2, SIZE) + val b2_copy = underTest.create(NAME_2, SIZE) + + assertThat(b1).isNotSameInstanceAs(b1_copy) + assertThat(b1).isNotSameInstanceAs(b2) + assertThat(b2).isNotSameInstanceAs(b2_copy) + } + + @Test + fun `getOrCreate - reuses instance`() { + val b1 = underTest.getOrCreate(NAME_1, SIZE) + val b1_copy = underTest.getOrCreate(NAME_1, SIZE) + val b2 = underTest.getOrCreate(NAME_2, SIZE) + val b2_copy = underTest.getOrCreate(NAME_2, SIZE) + + assertThat(b1).isSameInstanceAs(b1_copy) + assertThat(b2).isSameInstanceAs(b2_copy) + assertThat(b1).isNotSameInstanceAs(b2) + assertThat(b1_copy).isNotSameInstanceAs(b2_copy) + } + + companion object { + const val NAME_1 = "name 1" + const val NAME_2 = "name 2" + + const val SIZE = 8 + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt index 0da15e2399323..09589707b331d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt @@ -37,6 +37,7 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.log.table.TableLogBufferFactory import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectivityModel +import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.util.FakeMobileMappingsProxy import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger @@ -46,6 +47,7 @@ import com.android.systemui.util.mockito.eq import com.android.systemui.util.mockito.mock import com.android.systemui.util.mockito.whenever import com.android.systemui.util.settings.FakeSettings +import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -94,7 +96,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { } } - whenever(logBufferFactory.create(anyString(), anyInt())).thenAnswer { _ -> + whenever(logBufferFactory.getOrCreate(anyString(), anyInt())).thenAnswer { _ -> mock() } @@ -292,13 +294,13 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { // Get repos to trigger creation underTest.getRepoForSubId(SUB_1_ID) verify(logBufferFactory) - .create( + .getOrCreate( eq(MobileConnectionRepositoryImpl.tableBufferLogName(SUB_1_ID)), anyInt(), ) underTest.getRepoForSubId(SUB_2_ID) verify(logBufferFactory) - .create( + .getOrCreate( eq(MobileConnectionRepositoryImpl.tableBufferLogName(SUB_2_ID)), anyInt(), ) @@ -306,6 +308,46 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { job.cancel() } + @Test + fun `connection repository factory - reuses log buffers for same connection`() = + runBlocking(IMMEDIATE) { + val realLoggerFactory = TableLogBufferFactory(mock(), FakeSystemClock()) + + connectionFactory = + MobileConnectionRepositoryImpl.Factory( + fakeBroadcastDispatcher, + context = context, + telephonyManager = telephonyManager, + bgDispatcher = IMMEDIATE, + globalSettings = globalSettings, + logger = logger, + mobileMappingsProxy = mobileMappings, + scope = scope, + logFactory = realLoggerFactory, + ) + + // Create two connections for the same subId. Similar to if the connection appeared + // and disappeared from the connectionFactory's perspective + val connection1 = + connectionFactory.build( + 1, + NetworkNameModel.Default("default_name"), + "-", + underTest.globalMobileDataSettingChangedEvent, + ) + + val connection1_repeat = + connectionFactory.build( + 1, + NetworkNameModel.Default("default_name"), + "-", + underTest.globalMobileDataSettingChangedEvent, + ) + + assertThat(connection1.tableLogBuffer) + .isSameInstanceAs(connection1_repeat.tableLogBuffer) + } + @Test fun mobileConnectivity_default() { assertThat(underTest.defaultMobileNetworkConnectivity.value)