scope file observer to active Datastore.data observers [267792241]

Fixed

Internal Cleanup

Status Update

No update yet.

Description

yb...@google.com

created issue #1

Feb 3, 2023 08:32PM

ideally, we should limit file observer to the time frame where someone is observing the `datastore.data` so it is better scoped and doesn't do work unless necessary.

we already have versioning check to avoid issues there.

Comments

zh...@google.com <zh...@google.com> Aug 31, 2023 08:12PM

Reassigned to zh...@google.com.

zh...@google.com <zh...@google.com> #2Sep 5, 2023 05:46PM

Accepted by zh...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

yb...@google.com <yb...@google.com> #3Sep 6, 2023 11:47PM

No, this bug is about not listening to the file unless there is a collector on "datastore.data". That CL only handles stopping the file observer when the Datastore's Scope is cancelled, which we were already doing before that CL.

zh...@google.com <zh...@google.com> #4Sep 7, 2023 12:07AM

I see - as this is more of a performance improvement effort, how about we move it to datastore-1.1 stable so it doesn't block beta?

yb...@google.com <yb...@google.com> #5Sep 7, 2023 03:51PM

moved.

yb...@google.com <yb...@google.com> Feb 20, 2024 09:55PM

Assigned to yb...@google.com.

ap...@google.com <ap...@google.com> #6Feb 20, 2024 10:10PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 777ba8f9a1cdb4ca88263c6785c2708846b16d3f
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Feb 13 09:46:31 2024

Collect update notifications only when datastore is observed

Previously, DataStore started collecting updates from IPC right after
it is initialized and kept this collection alive as long as the scope
is alive.

This change updates it to only observe the file if there is an active
observer on the DataStore. We achieve this by updating `data` to a
channel flow that will collect on the shared flow when it is used.

Some tests needed to be updated because they made an assumption on the
order of execution which slightly changes with this channel flow but
does not effect correctness.

Fixes: 267792241
Test: SingleProcessDataStoreTest#observeFileOnlyWhenDatastoreIsObserved
Change-Id: Id8221718f75b9755119d5e9f38c4827fa907b867

M datastore/datastore-core/src/androidInstrumentedTest/kotlin/androidx/datastore/core/MultiProcessDataStoreSingleProcessTest.kt
M datastore/datastore-core/src/commonMain/kotlin/androidx/datastore/core/DataStoreImpl.kt
M datastore/datastore-core/src/commonTest/kotlin/androidx/datastore/core/CloseDownstreamOnCloseTest.kt
M datastore/datastore-core/src/commonTest/kotlin/androidx/datastore/core/SingleProcessDataStoreTest.kt

https://android-review.googlesource.com/2961704

th...@shareforce.eu <th...@shareforce.eu> #7Feb 21, 2024 01:08PM

In my code I keep a reference to the supervisorJob used in the datastore scope:

val job = SupervisorJob()
val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "dynamic name", CoroutineScope(Dispatchers.IO + job)

When the datastore is no longer in use, I call job.cancelAndJoin(). Does this change imply cancelling is no longer needed?

Message last modified on Feb 21, 2024 01:11PM

yb...@google.com <yb...@google.com> #8Feb 21, 2024 04:50PM

you still need to explicitly cancel if you want to re-open the same file via another instance. This is because StorageConnection objects still need to be explicitly closed and even when we are not observing the file, DataStore instance claims ownership of it.

There is a relevant FR about that here: b/221790279 . We do have plans to change it but we are not yet sure on the implementation. Ideally, we do want people keep these as singletons (since the file is a singleton on disk) but it does complicate certain use cases so we might allow multiple instances in the future.

th...@shareforce.eu <th...@shareforce.eu> #9Feb 22, 2024 01:42PM

Thank you for your quick answer and the relevant information, much appreciated.

na...@google.com <na...@google.com> #10Mar 6, 2024 07:49PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.datastore:datastore-core:1.1.0-beta02
androidx.datastore:datastore-core-android:1.1.0-beta02
androidx.datastore:datastore-core-iosarm64:1.1.0-beta02
androidx.datastore:datastore-core-iossimulatorarm64:1.1.0-beta02
androidx.datastore:datastore-core-iosx64:1.1.0-beta02
androidx.datastore:datastore-core-jvm:1.1.0-beta02
androidx.datastore:datastore-core-linuxx64:1.1.0-beta02
androidx.datastore:datastore-core-macosarm64:1.1.0-beta02
androidx.datastore:datastore-core-macosx64:1.1.0-beta02