Status Update
Comments
ma...@google.com <ma...@google.com>
to...@gmail.com <to...@gmail.com> #2
This is a particularly hard device to come by - do you happen to have access to the device? If so could you provide us with the output of: adb shell dumpsys media.camera > info.txt
Thanks!
an...@google.com <an...@google.com> #3
Stacktrace:
Caused by: java.lang.IllegalArgumentException: Can not get supported output size under supported maximum for the format: 34
at androidx.camera.camera2.internal.SupportedSurfaceCombination.getSupportedOutputSizes(SupportedSurfaceCombination.java:355)
at androidx.camera.camera2.internal.SupportedSurfaceCombination.getSuggestedResolutions(SupportedSurfaceCombination.java:197)
at androidx.camera.camera2.internal.Camera2DeviceSurfaceManager.getSuggestedResolutions(Camera2DeviceSurfaceManager.java:198)
at androidx.camera.core.CameraX.calculateSuggestedResolutions(CameraX.java:943)
at androidx.camera.core.CameraX.bindToLifecycle(CameraX.java:293)
at androidx.camera.lifecycle.ProcessCameraProvider.bindToLifecycle(ProcessCameraProvider.java:227)
Below are some findings based on our debugging
When Dex is connected
previewConfig.getMaxResolution() is returning "731x411" as maxSize.
Inside Preview.Builder.build() -> Default_MAX_resolution is set to "CameraX.getSurfaceManager().getPreviewSize()" which is 731x411
this is being picked as maxSize.
While rendering maxSize is 731x411 and minSize is 640x480 and below are available outputSizes
0 = {Size@11860} "4032x3024"
1 = {Size@11861} "3984x2988"
2 = {Size@11862} "4032x2268"
3 = {Size@11863} "3024x3024"
4 = {Size@11864} "2976x2976"
5 = {Size@11865} "3840x2160"
6 = {Size@11866} "3264x2448"
7 = {Size@11867} "4032x1960"
8 = {Size@11868} "2880x2160"
9 = {Size@11869} "3264x1836"
10 = {Size@11870} "2160x2160"
11 = {Size@11871} "2560x1440"
12 = {Size@11872} "2224x1080"
13 = {Size@11873} "2048x1152"
14 = {Size@11874} "1920x1080"
15 = {Size@11875} "1440x1080"
16 = {Size@11876} "1088x1088"
17 = {Size@11877} "1280x720"
18 = {Size@11878} "1024x768"
19 = {Size@11879} "1056x704"
20 = {Size@11880} "960x720"
21 = {Size@11881} "960x540"
22 = {Size@11882} "720x720"
23 = {Size@11883} "800x450"
24 = {Size@11884} "720x480"
25 = {Size@11885} "640x480"
26 = {Size@11886} "352x288"
27 = {Size@11887} "320x240"
28 = {Size@11888} "256x144"
29 = {Size@11889} "176x144"
and couldn't find any size in this range.
When Dex not connected
minsize = 640x480
maxsize = 1920x1080
0 = {Size@11836} "4032x3024"
1 = {Size@11837} "3984x2988"
2 = {Size@11838} "4032x2268"
3 = {Size@11839} "3024x3024"
4 = {Size@11840} "2976x2976"
5 = {Size@11841} "3840x2160"
6 = {Size@11842} "3264x2448"
7 = {Size@11843} "4032x1960"
8 = {Size@11844} "2880x2160"
9 = {Size@11845} "3264x1836"
10 = {Size@11846} "2160x2160"
11 = {Size@11847} "2560x1440"
12 = {Size@11848} "2224x1080"
13 = {Size@11849} "2048x1152"
14 = {Size@11850} "1920x1080"
15 = {Size@11851} "1440x1080"
16 = {Size@11852} "1088x1088"
17 = {Size@11853} "1280x720"
18 = {Size@11854} "1024x768"
19 = {Size@11855} "1056x704"
20 = {Size@11856} "960x720"
21 = {Size@11857} "960x540"
22 = {Size@11858} "720x720"
23 = {Size@11859} "800x450"
24 = {Size@11860} "720x480"
25 = {Size@11861} "640x480"
26 = {Size@11862} "352x288"
27 = {Size@11863} "320x240"
28 = {Size@11864} "256x144"
29 = {Size@11865} "176x144"
and we have 12 available sizes in this range
Camera2DeviceSurfaceManager.java:: getPreviewSize()
mCameraSupportedSurfaceCombinationMap.get(cameraId).getSurfaceDefinition().getPreviewSize() = "1920x1080"
cameraId=0
to...@gmail.com <to...@gmail.com> #4
The issue root cause is that CameraX will default filter out sizes smaller than 640x480. For Preview, the max size will be limited to under display size. I checked the HW spec info for the issue related devices. Display size of FUJITSU F-04J/F-05J is 360x640. That will result int that no size exists in the conditions that is larger or equal to 640x480 and smaller or equal to 360x640.
A temporary workaround for this situation is to use Preview.Builder#setTargetResolution() to set a size smaller than 640x480 to bypass the problem.
For device FUJITSU arrowsM04, I checked its HW spec info and its display size I found is 1280x720. It seems that the problem should not exist in the device.
Could you confirm that the problem exist on arrowsM04 device? What will be the returned value when using Display#getRealSize to obtain the display size?
an...@google.com <an...@google.com> #5
> A temporary workaround for this situation is to use Preview.Builder#setTargetResolution() to set a size smaller than 640x480 to bypass the problem.
OK. I will try it.
> Could you confirm that the problem exist on arrowsM04 device?
We receive the crash report (Crashlytics) that this crash has occurred on arrowsM04.
We don't have this device so we can't confirm that the problem really exist on arrowsM04.
> What will be the returned value when using Display#getRealSize to obtain the display size?
We can't investigate it for the same reason.
Thank you.
to...@gmail.com <to...@gmail.com> #6
This issue happened on devices that the display size is smaller than 640x480. In original auto-resolution mechanism, supported sizes smaller than 640x480 will be default filter out.
The auto-resolution mechanism encodes the guaranteed configurations tables in CameraDevice#createCaptureSession(SessionConfiguration). It defines that the PREVIEW size is the small one of the device display size and 1080p. The PREVIEW size will be the maximal size limitation for Preview use case. The reason it limits the size to display size and 1080p is the stream output in display size or 1080p has been able to provide good enough preview quality. Therefore, auto-resolution mechanism will limit the selected size to be smaller than the small one of the device display size and 1080p.
With above two conditions, in this issue, all sizes smaller than 640x480 have been filter out, therefore, there is no size smaller than the display size 320x240 can be selected to use. And cause the exception.
Solution:
When the display size is smaller than 640x480, auto-resolution mechanism won't filter out those small sizes smaller than 640x480. This makes those small size be left and can be selected for the Preview use case on small display devices.
The solution has been merged and will be included in next CameraX release.
to...@gmail.com <to...@gmail.com> #7
Hello.
This crash still occurs.
- CAMERAX VERSION: 1.0.0-beta4
- ANDROID OS BUILD NUMBER: Android 7.1.1
- DEVICE NAME: FUJITSU F-02H
We receive following crash report from FUJITSU F-02H. So far We have received this crash report only from F-02H.
java.lang.IllegalArgumentException
Can not get supported output size under supported maximum for the format: 34
androidx.camera.camera2.internal.SupportedSurfaceCombination.getSupportedOutputSizes (SupportedSurfaceCombination.java:349)
androidx.camera.camera2.internal.SupportedSurfaceCombination.getSuggestedResolutions (SupportedSurfaceCombination.java:197)
androidx.camera.camera2.internal.Camera2DeviceSurfaceManager.getSuggestedResolutions (Camera2DeviceSurfaceManager.java:198)
androidx.camera.core.CameraX.calculateSuggestedResolutions (CameraX.java:949)
androidx.camera.core.CameraX.bindToLifecycle (CameraX.java:351)
androidx.camera.lifecycle.ProcessCameraProvider.bindToLifecycle (ProcessCameraProvider.java:230)
(our application's package name).CameraFragment.bindCameraUseCases (CameraFragment.java:174)
to...@gmail.com <to...@gmail.com> #8
Could you help to provide the following information to clarify the issue?
1. Is the full name of the device Fujitsu Arrows NX F-02H that has a 1440x2560 display?
2. Please help to provide the supported output sizes of ImageFormat.PRIVATE that is obtained by StreamConfigurationMap#getOutputSizes(int).
an...@google.com <an...@google.com> #9
- Is the full name of the device Fujitsu Arrows NX F-02H that has a 1440x2560 display?
Yes
- Please help to provide the supported output sizes of ImageFormat.PRIVATE that is obtained by StreamConfigurationMap#getOutputSizes(int).
Since we don't have this device, we'll try to collect this information in the next version of our app. The next version will be released later this month.
to...@gmail.com <to...@gmail.com> #10
Hello.
- Please help to provide the supported output sizes of ImageFormat.PRIVATE that is obtained by StreamConfigurationMap#getOutputSizes(int).
We have collected the output of the device where the crash occurs.
Device1
- Model : arrows Be F-05J
- Android Version : 7.1.1
- Supported output sizes of ImageFormat.PRIVATE
CameraId 0: 480x480
CameraId 1: 2048x1536 ,1920x1080 ,1280x720 ,960x720 ,640x480 ,320x240 ,176x144
Device2
- Model : Fujitsu arrows M04
- Android Version : 7.1.1
- Supported output sizes of ImageFormat.PRIVATE
CameraId 0: 480x480
CameraId 1: 2048x1536 ,1920x1080 ,1280x720 ,960x720 ,640x480 ,320x240 ,176x144
Additional Information
CameraX version : 1.0.0-beta04
We collect the supported output sizes by following code.
val errorString = buildString {
append("The supported output sizes of ImageFormat.PRIVATE: ")
(requireContext().getSystemService(Context.CAMERA_SERVICE) as CameraManager).apply {
cameraIdList.forEachIndexed { index, cameraId ->
val msg = if (VERSION.SDK_INT >= VERSION_CODES.M) {
val configurationMap =
getCameraCharacteristics(cameraId).get(CameraCharacteristics.SCALER_STREAM_CONFIGURATION_MAP)
val sizes = configurationMap?.getOutputSizes(ImageFormat.PRIVATE)
"CameraId $index: ${sizes?.joinToString(" ,")}"
} else {
"CameraId $index: This device version is under M."
}
append(msg)
}
}
}
to...@gmail.com <to...@gmail.com> #11
an...@google.com <an...@google.com> #12
I tried to find the device specs and both 720x1280
size display. For the camera id 0 device, it is a different case that the display size is larger than 640x480
but the device only supports a 480x480
size. The case also caused the same IllegalArgumentException and was also fixed by 1.0.0-beta04
release. Before 480x480
size would be filtered out and then caused the IllegalArgumentException. After it was merged, the 640x480
size threshold was removed and then the 480x480
size would be kept and selected to use.
It looks like 1.0.0-beta04
release had been used to collect the supported sizes information. But the issue should have been fixed by 1.0.0-beta04
release. Did you only check the device model name to collect the supported sizes information or collect the information when the IllegalArgumentException issue happens again?
CameraX's 1.0.0-beta04
version. Maybe you can also consider to upgrade to the latest 1.0.0-rc01
version for your application. Thanks.
to...@gmail.com <to...@gmail.com> #13
Did you only check the device model name to collect the supported sizes information or collect the information when the IllegalArgumentException issue happens again?
We collect informations only from the device on which IllegalArgumentException happened.
Our latest app uses CameraX version 1.0.0-beta10
and this issue still occurres.
However we don't receive crash report from Fujitsu arrows Be F-05J
or Fujitsu arrows M04
so far. (This doesn't mean this issue is fixed on these devices because our app is heavily rely on camera so these device's user wouldn't use our app anymore.)
Instead, we receive crash report from
- Model : Fujitsu F-03K
- Android Version : 7.1.2
- Supported output sizes of ImageFormat.PRIVATE
CameraId 0 : 480x480
CameraId 1 : 2048x1536 ,1920x1080 ,1280x720 ,960x720 ,640x480 ,320x240 ,176x144
to...@gmail.com <to...@gmail.com> #14
I missed some settings when I simulated the issue by robolectric test so that I was not able to reproduce it. Now, I can reproduce the issue if the device only supports one 480x480 resolution. I'm working on the solution and target to make it included in next release.
to...@gmail.com <to...@gmail.com> #15
Branch: androidx-main
commit 69d15dff7bb857ee33a0f643ff42a0f8bc475ab2
Author: charcoalchen <charcoalchen@google.com>
Date: Fri Jan 08 18:30:03 2021
Fixed IllegalArgumentException issue happened when all preview supported sizes are smaller than 640x480 and display size is larger than 640x480.
Do not filter out sizes smaller than 640x480 when all preview supported sizes are smaller than 640x480 and display size is larger than 640x480.
Relnote:"Fixed IllegalArgumentException issue happened when all preview supported sizes are smaller than 640x480 and display size is larger than 640x480."
Bug: 150506192
Test: SupportedSurfaceCombinationTest
Change-Id: I2a63ce8e2ad42a9cc060c8635ac3603bf440b1ec
M camera/camera-camera2/src/main/java/androidx/camera/camera2/internal/SupportedSurfaceCombination.java
M camera/camera-camera2/src/test/java/androidx/camera/camera2/internal/SupportedSurfaceCombinationTest.java
to...@gmail.com <to...@gmail.com> #16
an...@google.com <an...@google.com> #17
Can confirm your sample project is crashing for me, thanks. I will investigate it
to...@gmail.com <to...@gmail.com> #18
Nice.
Tell me if you need more data from the real app, the repro is a bit hackish but leads to same result so I suppose it's still valid enough about the root cause.
an...@google.com <an...@google.com> #19
Chuck, I need your help with investigating this. The smallest repro
var reload by remember { mutableStateOf(0) }
LaunchedEffect(Unit) {
while (true) {
reload++
delay(100)
}
}
val items = remember(reload) {
derivedStateOf {
List(1000000) { it }
}
}
Text("List of size ${items.value.size}")
After a few seconds the app will crash with out of memory exception. Seems like there is some kind of a memory leak in the derivedstate implementation.
to...@gmail.com <to...@gmail.com> #20
For the record the main app crash when scrolling the lazy list without any refresh of any derivedState. So if no derived inside Lazy it's maybe 2 different issues and I repro the wrong one.
an...@google.com <an...@google.com> #21
We use DerivedState as an implementation detail in lazy lists as well, so most likely it is the same issue. Otherwise we first try to fix DerivedState and then test with your app again
ch...@google.com <ch...@google.com> #22
The issue is that there is an assumption that between the start of a function and the end that the Snapshot.current.id
does not change. However, reading a derivedStateOf
value will change the id of a snapshot so the derived state of value can know when to correctly recalculate the value if the values to calculate it change in the same snapshot.
This causes a memory leak because the RecomposeScope
tries to only forget about object that it is no longer observing if it the changes are actually applied for that scope. It does this by recording a lambda in the changes
list of composition. It further checks if the change that is being applied is from the last composition, if not, the next composition will clean the scope so it can ignore the notification. The recompose scope uses the Snapshot.id
of the composition snapshot as a token to identify which composition it is in. This is not valid to do as the snapshot id
can change during the composition. Because it changed the RecomposeScope
thinks it is receiving an out-of-date notification to forget its unused objects and ignores the notification.
The fix for this is uncertain yet. We can either copy the snapshot id at the beginning of composition and use that instead of snapshot.id
or always assume that start()
and end()
for the recompose scope are paired and trust that when end()
of the RecomposeScope is called it is for the same composition for which start()
was called. The check might also not be needed as the case it is checking probably never occurs.
ch...@google.com <ch...@google.com> #23
I have a fix for this pending that takes the "copy the snapshot id at the beginning of composition" option from above.
to...@gmail.com <to...@gmail.com> #24
Nice, is this 1.2 material or beta is already branched and it's 1.3?
ch...@google.com <ch...@google.com> #25
This should make it into 1.2 automatically. If not, i will start a cherry-pick to get it into 1.2.
ap...@google.com <ap...@google.com> #26
Branch: androidx-main
commit 78dc85f30cb72ba38d36bee4f85509a975f19d06
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Wed May 04 12:01:08 2022
Fix memory leak when observing derived state objects
When observing derived state objects, if the recompose snapshot changed
id it caused the derived state objects no longer observed by composition
to be retained by the recompose scope, causing them to leak. The
snapshot id would change if the read is the first time a derived state
object is read in the snapshot.
To detect when a derived state object is no longer necessary a token
value is used to record when the derived state was added into the
composition. This token was the current snapshot id. However, since
the id is not static through-out composition, this could cause the
notification to be skipped.
This change fixes the token to be the snapshot id at the beginning of
composition and, after that, ignores the snapshot id. This means the
token is consistent through-out the same composition allowing unused
derived object to be detected.
Fixes 230168389
Test: new test, ./gradlew :compose:r:r:tDUT
Change-Id: I877c826764592a7e424667d3e2b677d385cf0b7f
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/CompositionTest.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/CompositionAndDerivedStateTests.kt
to...@gmail.com <to...@gmail.com> #27
So just tested with Snapshot 8544662 that contains that fix.
And the root issue is still present. The application attached earlier still crash after a few seconds. (And my main app during simple scrolls).
Can you please reopen?
an...@google.com <an...@google.com>
to...@gmail.com <to...@gmail.com> #28
Thanks,
For the record on release builds there's a huge win I can scroll a lot longer than without this so this was also related I guess.
an...@google.com <an...@google.com> #29
```
var reload by remember { mutableStateOf(0) }
LaunchedEffect(Unit) {
while (true) {
reload++
delay(100)
}
}
val items = remember(reload) {
derivedStateOf {
List(1000000) { it }
}
}
LazyColumn {
items(items.value) { item ->
Text("item $item")
}
}
```
Wasn't able to decouple it even more yet. Probably what is important is the fact that the lambda passed into LazyColumn is wrapped into another derived state and then this derived state is executed in composition, but when I tried to emulate it it wasn't crashing
ch...@google.com <ch...@google.com> #30
The previous fix was necessary but not sufficient to fix this issue.
The issue demonstrated by the above is when derivedStateOf
object changes what objects it reads the composition still remembers the old objects in addition to the new objects. A composition maintains a map of the objects that a dependency object reads so that, when one of those objects changes, it knows which dependency object to invalidate. If this list changes the the only the current dependencies should be retained and any previous value no longer observed should be forgotten.
I am investigating ways to ensure the inverted map is valid without causing undo additional overhead.
to...@gmail.com <to...@gmail.com> #31
Small bump on this one, the app is now in prod and I start to get OOM crash reports.
Is this something that could be targeted for 1.2 or should I try to figure out a way to reduce even more the data used in those derivedstate for the moment?
ap...@google.com <ap...@google.com> #32
Branch: androidx-main
commit ad7c95cd58204ef7be4cec4f999d602619c51a8a
Author: Andrei Shikov <ashikov@google.com>
Date: Wed Jun 01 14:11:05 2022
Clean up derived state dependencies in composition
Fixes two cases when derived state dependencies were retained:
1) Derived state instance is no longer observed in composition.
2) Derived state instance is still observed in composition, but its dependencies has been changed since last observation.
As composition uses `dependency -> derived state` mapping, this fix relies on iterating over all map values with `.removeValueIf`. In the future, we can optimize the lookup by using reverse index to lookup dependencies for derived state.
Fixes: 230168389
Test: CompositionAndDerivedStateTests#changingTheDerivedStateInstanceShouldClearDependencies, CompositionAndDerivedStateTests#changingDerivedStateDependenciesShouldClearThem
Change-Id: Id175952fedae53d669d028e460c65d6d2d5d4174
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/collection/IdentityScopeMap.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/CompositionAndDerivedStateTests.kt
to...@gmail.com <to...@gmail.com> #33
Ok so the reproduction posted here is now fixed, but the main app still OOM when scrolling in large lazy list can this be reopened and moved back to lazylist?
Starting to wonder if now it's not back to be tied to
to...@gmail.com <to...@gmail.com> #34
The heapdump looks pretty much the same see attached. Will need some hints to build the new repro.
ad...@google.com <ad...@google.com> #35
Cases in
an...@google.com <an...@google.com>
an...@google.com <an...@google.com> #36
The best new repro I found so far is:
var reload by remember { mutableStateOf(0) }
LaunchedEffect(Unit) {
while (true) {
reload++
delay(200)
}
}
val items = remember {
derivedStateOf {
reload
ShortArray(10000000) { -5 }
}
}
LazyColumn {
items(500) { item ->
Text("item index=$item ${items.value[0]}")
}
}
But now it will not crash straightaway because of the previous fix and now you have to scroll until it crashes. Note: I used ShortArray so it is easier to find it in the heap as short[]
to...@gmail.com <to...@gmail.com> #37
Thanks for the repro it made me think about testing a change on my code and reverting the previous derived state indexer to avoid passing the full items list to items() and it does reduce the memory constraint (it still crash but requires more scroll).
So going back from
val indexer by remember {
derivedStateOf {
List(lazyPagingItems.itemCount) { it }
}
}
items(indexer) { index ->
val item = lazyPagingItems[index]
to the normal
items(lazyPagingItems) { item ->
Does reduce the issue when before the 2 derivedstate fixes it was helping a lot.
to...@gmail.com <to...@gmail.com> #38
Sorry to post here as not really related but I'm trying to update the snapshot version to a more recent version to see if your
But I'm getting java.lang.NoClassDefFoundError: Failed resolution of: Landroidx/compose/foundation/lazy/LazyItemScope$DefaultImpls;
errors from any libraries compiled with compose 1.2 beta 2 probably because of
Is there any trick to have this working or do I need to wait for the libs to update and won't be able to test possible fixes for this issue here? (I tested adding -Xjvm-default=all
on my side side but no effect)
an...@google.com <an...@google.com> #39
Does reduce the issue when before the 2 derivedstate fixes it was helping a lot. Yes, I would recommend you to rewrite your code without derivedstate in the meantime
see if your
https://github.com/androidx/androidx/commit/a1ba9c954b9cc2e36bf897e4c649f52442a07a0b fix the rarecrash I get. No, I do not expect this change to fix anything for you https://issuetracker.google.com/issues/233929916
About your new crash I will let the responsible folks know, thanks
to...@gmail.com <to...@gmail.com> #40
Yes, I would recommend you to rewrite your code without derivedstate in the meantime
Is there any performance difference between LazyVerticalGrid with 1 column and LazyColumn ?
I still use a derivedstate to manually window and implement the grid via a LazyColumn as the app allows switching between both and I need to listen to the state for other things related to first visible items.
Since LazyGrid and LazyColumn state are not equivalent and there's no common interface for the common functions it's a pain to deal with both and I'd like to stay with only one.
an...@google.com <an...@google.com> #41
If your state is not frequently changing (like when user tap on switch mode button, not on every frame) you don't really need derived state.
```
val mode by remember { mutableStateOf(true) }
val derivedValue = remember(mode) {
... calculate something based on mode
}
```
to...@gmail.com <to...@gmail.com> #42
By derived data I meant the data to feed the LazyColumn to emulate the LazyGrid.
val items = remember(lazyPagingItems.itemCount, stepSize) {
derivedStateOf {
List(lazyPagingItems.itemCount) { it }.windowed(stepSize, stepSize, true)
}
}
But yes I suppose the preventive derivedStateOf is not mandatory in this case.
The problem about the state being different classes, is for the fastscroll for example, or the quick return or any other visible items observing to react. The state is hoisted and used elsewhere, but this does not work if the inner component can change between both, this is the main pain point of the 2 different states, either have 2 different implementations of the observers or build another state and wrap what's needed.
Anyway this is not the place and since the LazyGrid api is no more experimental and I believe you about perf, I'll migrate all to LazyGrid to simplify my future life.
Thanks again.
to...@gmail.com <to...@gmail.com> #44
Forget to post but yes this fix the compilation.
But since I digressed a lot, just wanted to be sure the original issue is still being investigated? I see it's already 1.3 now.
th...@gmail.com <th...@gmail.com> #45
th...@gmail.com <th...@gmail.com> #46
ap...@google.com <ap...@google.com> #47
Branch: androidx-main
commit 11dbe1f459907ec1c23e730c77e33de39fc894cf
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Wed Jun 22 14:17:20 2022
Remove tracking information from release recompose scopes
Recompose scopes can be tracked by the recomposer past when they
are removed from composition. Since it is expensive to accurately
track the lifetime of scopes they are cleaned up lazily. This
lazy cleanup might not trigger in some cases causing the references
tracked by the scope to leak.
This change first removes tracked references from recmpose scopes
that are removed from composition and, second, reduces the chance
that such scopes are tracked longer then necessary by cleaning up
the conditionally invalid scopes table when scopes are removed.
Fixes: 230168389
Test: ./gradlew :compose:r:r:tDUT
Change-Id: Idfe3e3d5d6d65bac2cf06ee19fd00a5d9253d8a1
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composition.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/RecomposeScopeImpl.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/CompositionAndDerivedStateTests.kt
to...@gmail.com <to...@gmail.com> #48
So unfortunately it's not 100% fixed, can this be reopened again?
The repro app still crash after scrolling but requires more than before. (Same for prod app)
Furthermore last snapshot completely breaks LazyX overscroll (a LazyColumn will now overscroll on left / right too when 100% unwanted see attached video).
an...@google.com <an...@google.com> #49
Thanks for the report. I filed a separate bug for this issue with overscroll -
to...@gmail.com <to...@gmail.com> #50
Thanks I lack time before the holidays but I hope you can go to the bottom of it. (Other bug is private I can't follow it)
Since the leak is not tied to the actual composable even leaving the screen completely won't return the memory so in the end if the user stays enough in the app it will reach OOM, and even if not seems on Pixels the memory killer don't like it at all and kill the app even with a foreground service active for low memory when going background. (And well that's a bummer to have a music player killed during playback it kinda defeat it's purpose).
an...@google.com <an...@google.com> #51
Sorry, made
an...@google.com <an...@google.com> #52
I tried to run the demo app again after all the recent changes and unfortunately can't reproduce what you are seeing. It is not crashing for me after scroll and I don't see anything leaking. And regardless of the leaks, I really do not recommend you writing code like this
val items = remember(lazyPagingItems.size, stepSize, reload) {
derivedStateOf {
List(lazyPagingItems.size) { it }.windowed(stepSize, stepSize, true)
}
}
Allocating such lists only to produce list of indexes is very inefficient. You can just instead write some function which will be mapping indexes from the main data set to the new windowed one and it will require no allocations. But realistically as we already discussed you better just use LazyVerticalGrid, it is an api exactly for this use case. If after applying those recommendations you will think you still have some memory leaks feel free to file a separate bug. Thanks
to...@gmail.com <to...@gmail.com> #53
The issue also occurs on normal lazylist without using those derived state.
The demo requires a lot more scroll but will trigger the issue at some point.
The GC will say 75% free then 45% free then 75% free then at some point will go down to 15% then 45% then ... Until the OOM.
This is just no more automatic and quick as previously. Scroll more up down to at least the middle to trigger it.
an...@google.com <an...@google.com> #54
For you it is crashing exactly because you are creating huge lists inside derivedStateOf. And I recommend you to not do that in your code as you are really don't have to
to...@gmail.com <to...@gmail.com> #55
Again I'm not when in list mode I directly use the lazy items and it behave the same.
And even when I do I create a list of 22000 Int this is not exactly what you can call a huge list.
Anyway I'll try to build another repro when back in 2 weeks. Will link here for proper attribution.
to...@gmail.com <to...@gmail.com> #56
Just tested again with one of the last snapshot from last hours and there's changes.
It's now quite hard to reproduce with the repro from here but still randomly can trigger.
But the very nice change is that removing the lazycolumn from composition does free the memory now. So on the main app the impact should be minimal instead of ineluctable. Just need the overscroll fix.
Was the video enough or need something before I leave ?
to...@gmail.com <to...@gmail.com> #57
Did a quick heap dump since the memory is freed on composable removal from the screen and the data is different see attached screenshots.
Maybe this will lead some light until I can build a repro in at least 2 weeks.
to...@gmail.com <to...@gmail.com> #58
One last for Adam
java.lang.NoClassDefFoundError: Failed resolution of: Landroidx/compose/foundation/lazy/grid/LazyGridItemScope$DefaultImpls;
Seems that one passed the 2 JvmDefaultWithCompatibility fixes.
And now sea and sun.
to...@gmail.com <to...@gmail.com> #59
So for the record my leaks are now fully fixed with snapshot 8859449 (Maybe thanks to this
But I'm now facing another strange Compose bug that I have an hard time debugging :(
ma...@gmail.com <ma...@gmail.com> #60
This issue should be reopened as it still valid in compose 1.3.0-beta01
.
How to reproduce:
1 - Scroll the LazyList
a bit
2 - Press the big red rectangle to remove the LazyList
from the composition
3 - Press the big red rectangle again
4 - Go to 1
The memory usage will grow up endlessly.
to...@gmail.com <to...@gmail.com> #61
I can't repro with my previous code and have not found a proper repro (not tested #60)
But I've pushed a beta of my app with 1.3.0 beta 1 and I've now got a return of OOM crashes, so there's definitively something back here :(
ch...@google.com <ch...@google.com>
ch...@google.com <ch...@google.com> #62
I reopen the bug. I reproduced the memory leak demonstrated by the above linked MainActivity.kt supplied by
SnapshotStateObserver
created for AndroidComposeView
are not be removed when the LazyColumn
is removed from composition.
This appears to be a different issue that was originally reported but will use this bug to track this as it is still a memory leak involving a LazyColumn
or LazyRow
and might be the issues the followers of this bug are running into instead of the original problem.
Note that a number of fixes regarding this went into 1.2 and 1.3. However, if you just update the version of androidx.compose.ui to 1.3.0-beta01 (which is what you get if you update compose_ui_version
in the default Studio template to 1.3.0-beta01) you will not many of these fixes as they are part of the 1.3 runtime. Ensure you update to an explicit reference to androidx.compose.runtime:1.3.0-beta01 as 1.3.0-beta01 only requires you update to 1.2.0+ of the runtime and will not automatically update to the latest runtime.
to...@gmail.com <to...@gmail.com> #63
For the record the mails are hidden to normal users you should not disclose them in case it's unwanted and to avoid spam.
I'm full B1 and no issue with alpha3 all previous issues where correctly fixed.
I proposed to Andrey to bisec between those versions is this still necessary?
an...@google.com <an...@google.com> #64
ch...@google.com <ch...@google.com>
mo...@google.com <mo...@google.com> #65
Ralston is making a large change for focus that will fix this problem. He's agreed to take this on.
da...@sonos.com <da...@sonos.com> #66
I had the same type of issues, where using a LazyRow and scrolling a lot would see performance degradation to the point where the app was unusable. I can say that the fixes from this bug have definitely fixed my issue, so thank you very much! However, now I'm seeing that my derivedStateOf calculations are getting called all the time.
I have a derivedStateOf which calculates layout information based off changes to the scroll state. For example:
class LayoutProvider(
val layoutState: State<MyLayout>
)
@Composable
fun rememberLayoutProvider(): LayoutProvider {
val layoutState = remember {
derivedStateOf {
// run some big calculations to calculate a layout
calculateLayout()
}
}
return remember { LayoutProvider(layoutState) }
}
And then many different composables use my derived state to create their own derivedStateOf to get their layout information. For example:
@Composable
fun SomeTile() {
val myLayout = remember {
derivedStateOf {
layoutProvider.getMyLayout()
}
}
do something with myLayout
}
In compose 1.2.0, this worked really well, and when the scroll state would change, the layout provider's calculation would run once, and only composables whose specific layout changed (myLayout
) would get recomposed.
Now in compose 1.3.0-beta01, the calculation in the LayoutProvider gets called a large number of times, and the composoables who have their own derivedState get recomposed even when their specific layout hasn't changed.
I noticed the change to the derivedStateOf
, with the addition of a policy, but I tried writing my own and still couldn't get the results I wanted. Can you please explain how I can go about getting these calculations to get called as I'd expect and only have the composables that have their derived state changed recompose?
Thank you
da...@sonos.com <da...@sonos.com> #67
To give some more information, when when calling derivedStateOf(structuralEqualityPolicy())
in both those places in the code above, it still behaves incorrectly. Basically, the composable SomeTile
always gets recomposed any time the scroll state changes, and only at the call site where myLayout
gets used does the derivedState calculation run (i.e. layoutProvider.getMyLayout()
. Seeing the whole point of derivedState is so that we can avoid recomposing composables on state changes that can happen frequently, this now feels broken to me. Please let me know if I'm missing something.
Thank you
da...@sonos.com <da...@sonos.com> #68
I was able to make it even more simple:
@Composable
fun SomeComposable(
scrollState: LazyListState
) {
val red by remember {
derivedStateOf(structuralEqualityPolicy()) {
// cause an observation on the scroll state
scrollState.firstVisibleItemScrollOffset != 0
}
}
Log.e("Test", "composing SomeComposable")
if (red) {
Log.e("Test", "composing a red box")
Box(modifier = Modifier.background(Color.Red).fillMaxSize())
}
else {
Log.e("Test", "composing a blue box")
Box(modifier = Modifier.background(Color.Blue).fillMaxSize())
}
}
With this code, red
should always be true except when the scroll offset is at 0, and thus the composable should only ever recompose when the scroll offset comes to zero or leaves zero. However, it recomposes on every single scroll change.
to...@gmail.com <to...@gmail.com> #69
Your last example sounds normal IMO. The scrollstate change, the SomeComposable is recomposed the state is calculated, if is called and your log is ran.
To see any benefit here you need to extract the second part to another composable that is skippable.
@Composable
fun SomeComposable(
scrollState: LazyListState
) {
val red by remember {
derivedStateOf(structuralEqualityPolicy()) {
// cause an observation on the scroll state
scrollState.firstVisibleItemScrollOffset != 0
}
}
Log.e("Test", "composing SomeComposable")
SomeComposable2(red)
}
@Composable
fun SomeComposable2(
red: Boolean
) {
if (red) {
Log.e("Test", "composing a red box")
Box(modifier = Modifier.background(Color.Red).fillMaxSize())
}
else {
Log.e("Test", "composing a blue box")
Box(modifier = Modifier.background(Color.Blue).fillMaxSize())
}
}
This will display the first composing log each time the scrollstate change, but the box composing will only happen when the derived state change as the SomeComposable2 is skippable.
da...@sonos.com <da...@sonos.com> #70
Thanks for the response, but unfortunately, that still isn't correct IMO. I understand the concept of using a composable that is skippable, but what that would accomplish is that when red
changes, SomeComposable
would not recompose, while SomeComposable2
would. That is not what I'm trying to accomplish. The issue here is that red
is not supposed to change unless the scrollable state crosses the zero boundary. So in your example, SomeComposable2
still recomposes every single time the scroll state changes, but it shouldn't.
The issue is that the red
state only gets evaluated/calculated when it is accessed, which is in the recomposition, but at this point, it's too late, as the composable is already recomposing. So DerivedState is no longer useful. In Compose 1.2.0, the red
calculation would run at the time that the scroll state changes outside the composition scope, and then would appropriately cause the composable to recompose only if the red
state actually changed.
to...@gmail.com <to...@gmail.com> #71
No in my example SomeComposable2 does not recompose because it's skippable and red state does not change.
SomeComposable will always recompose because the scrollstate change. It's up to you that your composable is made of skippable sub component so that they are not recomposed.
In the case of #68 the if is not a composable function so is not skippable. So if SomeComposable is recomposed the if will always be ran and so your log will run. If you had something different before then it was something else and not how compose is supposed to work.
In all case in my example the red state is read as it's a parameter to SomeComposable2 so it's up to date thanks to the derivedstate and so will properly change.
I use this pattern a lot in prod and it works like that since 1.0 and it still works the same in 1.3 alpha3 (and beta 1 but I'm not in prod with it due to the return of the mem leak).
In all cases your issue seems unrelated to this one so you might want to open another one if you want more traction from Google.
da...@sonos.com <da...@sonos.com> #72
da...@sonos.com <da...@sonos.com> #73
da...@sonos.com <da...@sonos.com> #74
Here is sample code that describes exactly the issue:
class MainActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
// A surface container using the 'background' color from the theme
Surface(
modifier = Modifier.fillMaxSize(),
color = MaterialTheme.colors.background
) {
OuterComposable()
}
}
}
}
@Suppress("EqualsOrHashCode")
class ZeroMap(
val zeroMap: Map<Int, Boolean>
) {
override fun equals(other: Any?): Boolean {
if (this === other) {
Log.e("StateTest", "ZeroMap is the same reference")
return true
}
if (javaClass != other?.javaClass) {
Log.e("StateTest", "ZeroMap is not the same because of javaClass")
return false
}
other as ZeroMap
if (zeroMap.size != other.zeroMap.size) {
Log.e("StateTest", "ZeroMap is not the same because trackLayouts.size (${zeroMap.size}) != other.trackLayouts.size (${other.zeroMap.size})")
return false
}
val otherEntries = other.zeroMap.entries
zeroMap.entries.forEachIndexed { index, entry ->
val otherEntry = otherEntries.elementAt(index)
if (entry.value != otherEntry.value) {
Log.e("StateTest", "ZeroMap is not the same because entry.value (${entry.value}) != otherEntry.value (${otherEntry.value}) for index $index")
return false
}
}
Log.e("StateTest", "ZeroMap is the same value")
return true
}
}
class ZeroStateProvider(
val scrollState: LazyListState,
private val zeroState: State<ZeroMap>
) {
fun isAtZero(index: Int) = zeroState.value.zeroMap[index]
}
@Composable
fun rememberZeroStateProvider(): ZeroStateProvider {
val scrollState = rememberLazyListState()
val zeroState = remember {
derivedStateOf(structuralEqualityPolicy()) {
Log.e("StateTest", "calculating zero state for scroll offset: ${scrollState.firstVisibleItemScrollOffset}")
val zeroMap = mutableMapOf<Int, Boolean>()
scrollState.layoutInfo.visibleItemsInfo.forEach {
zeroMap[it.index] = it.offset == 0
}
ZeroMap(zeroMap)
}
}
return remember {
ZeroStateProvider(scrollState, zeroState)
}
}
@Composable
fun OuterComposable(
modifier: Modifier = Modifier,
) {
Log.e("StateTest", "Composing OuterComposable")
val zeroStateProvider = rememberZeroStateProvider()
LazyRow(
state = zeroStateProvider.scrollState,
horizontalArrangement = Arrangement.spacedBy(60.dp),
verticalAlignment = Alignment.Top,
modifier = modifier
.fillMaxWidth()
) {
items(
count = 100,
) { index ->
InnerComposable(index, zeroStateProvider)
}
}
}
@Composable
private fun InnerComposable(
index: Int,
zeroStateProvider: ZeroStateProvider,
modifier: Modifier = Modifier
) {
Log.e("StateTest", "Composing InnerComposable (index=$index)")
val color by remember {
derivedStateOf(structuralEqualityPolicy()) {
Log.e("StateTest", "InnerComposable: calculating color")
when (zeroStateProvider.isAtZero(index)) {
true -> Color.Red
false,
null -> Color.Blue
}
}
}
Box(
modifier = modifier
.background(color)
.size(200.dp)
)
}
The output in Compose 1.2.0 gives:
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 1
E/StateTest: Composing InnerComposable (index=0)
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: Composing InnerComposable (index=4)
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 3
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 5
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 6
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
and in Compose 1.3.0-beta01 gives:
E/StateTest: calculating zero state for scroll offset: 2
E/StateTest: ZeroMap is not the same because entry.value (false) != otherEntry.value (true) for index 0
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 2
E/StateTest: ZeroMap is not the same because entry.value (false) != otherEntry.value (true) for index 0
E/StateTest: Composing InnerComposable (index=0)
E/StateTest: calculating zero state for scroll offset: 2
E/StateTest: ZeroMap is not the same because entry.value (false) != otherEntry.value (true) for index 0
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 2
E/StateTest: ZeroMap is not the same because entry.value (false) != otherEntry.value (true) for index 0
E/StateTest: calculating zero state for scroll offset: 2
E/StateTest: ZeroMap is not the same because entry.value (false) != otherEntry.value (true) for index 0
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: InnerComposable: calculating color
E/StateTest: calculating zero state for scroll offset: 4
E/StateTest: ZeroMap is the same value
E/StateTest: calculating zero state for scroll offset: 5
E/StateTest: ZeroMap is the same value
As you can see from this output, in Compose 1.2.0, calculating zero state for scroll offset
only appears once every time the scroll state changes, and then the inner compositions recalculate accordingly. But in Compose 1.3.0-beta01, calculating zero state for scroll offset
shows up many times every time the scroll state changes. The number of times it runs increases with the number of composables that use its state.
From the documentation of derivedStateOf, it says "The result of calculation will be cached in such a way that calling [State.value] repeatedly will not cause[calculation] to be executed multiple times". This was true prior to Compose 1.3.0-beta01, but no longer is.
Please let me know what you think. Thanks!
to...@gmail.com <to...@gmail.com> #75
As said I'm not a Googler and your issue is unrelated to the original issue. You really should open another issue.
I'd say that your issue might not be related to derivedstate at all but to the compiler generation of your ZeroStateProvider class.
I'd use
da...@sonos.com <da...@sonos.com> #76
Yes, I'm ok with creating a new issue (which I did here:
to...@gmail.com <to...@gmail.com> #77
You can easily bisect the version that introduced the issue. All the fixes for this issue are in 1.2.0. AFAIR the only missing to fix all was
Try 1.3 alpha 3 that I use in prod with no performance regression. Then alpha 2 then 1 ;)
da...@sonos.com <da...@sonos.com> #78
It was introduced by
to...@gmail.com <to...@gmail.com> #79
Small bump on this to follow from #65
Is this planned to be backported to 1.3?
With Material 3 pinned to 1.3 we can't keep 1.3 A3 and update material. And not sure they will start an 1.1 alpha pinned to 1.4 before a final 1.3 release. (Maybe you have some info about that)
to...@gmail.com <to...@gmail.com> #80
So 1.3 RC1 is cut, is it possible to have some insight on this please? On my app with tons of large lazylist this leads to very fast OOM (even faster than previous issues).
Not having a solution for 1.3 may have high impact on updating M3 / Glance and the sooner we know the easier to anticipate.
ad...@google.com <ad...@google.com> #81
1.3-rc01 hasn't been cut yet, though the commit to bump the version to rc is in the system to ready for that build cut
ap...@google.com <ap...@google.com> #82
Branch: androidx-main
commit 3bd044a0a148b16ce17eaa71ceb5b8aed1bde70a
Author: Ralston Da Silva <ralu@google.com>
Date: Thu Sep 15 13:22:36 2022
Fix for issue where ModifierLocals were not being updated when modifiers were reused
With the introduction of Modifier.Node in aosp/2108724 onModifierLocalsUpdated was
not being called with the default value when modifiers were reused. This broke
LazyListFocusMoveTest.
Verified that this CL fixes the test, and added new tests to ModifierLocalSameLayoutNodeTest.kt
Bug: 245512265
Bug: 245131728
Bug: 245363143
Bug: 230168389
Fixes: 245512265
Fixes: 245131728
Test: ./gradlew compose:f:f:cC -P android.testInstrumentationRunnerArguments.class=androidx.compose.foundation.lazy.list.LazyListFocusMoveTest
Test: ./gradlew compose:ui:ui:cC -P android.testInstrumentationRunnerArguments.class=androidx.compose.ui.modifier.ModifierLocalSameLayoutNodeTest
Change-Id: I90d10bf341c4b19a979440886f3d002f5bc8320a
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/modifier/ModifierLocalSameLayoutNodeTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/BackwardsCompatNode.kt
to...@gmail.com <to...@gmail.com> #83
So tested snapshot 9082024 that include this and it seems to be fixed.
And I also see nice scrolling performance on large lists, maybe due to other commits.
The last question is. Are there any commits in 9082024 that won't be part of the final 1.3? Or can I expect the same performance gain in the coming 1.3 releases ?
an...@google.com <an...@google.com> #84
ra...@google.com <ra...@google.com>
ju...@gmail.com <ju...@gmail.com> #85
na...@google.com <na...@google.com> #86
This bug was linked in a change in the following release(s):
androidx.compose.ui:ui:1.3.0-rc01
na...@google.com <na...@google.com> #87
The following release(s) address this bug:
androidx.compose.runtime:runtime:1.3.0
Description
Jetpack Compose version: 1.2.0 alpha 07 Jetpack Compose component used: LazyColumn Android Studio Build: AGP 7.3.0-alpha09 Kotlin version: 1.6.20 / 1.6.21
I have not yet a repro but will need some input about where to look for to build it.
Starting recently so I guess Alpha 06 or 07 I started to have OOM issues in my app. Doing some heap dump I see the following see attached images.
The first dump is after a few scroll and normal usage of the application. The second dump is after a lot more scrolling in a single LazyColumn.
As you can see the retained size is insane for SnapshotMutableStateImpl and the app quickly OOM.
I'd need some input about what is stored in those to be able to build a repro