Fixed
Status Update
No update yet.
Comments
cs...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023
Convert APIs using PointF to use Float instead
PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.
Also fixed bug with unclosed paths causing discontinuity at the
start/end point.
Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e
M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt
https://android-review.googlesource.com/2649119
Branch: androidx-main
commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023
Convert APIs using PointF to use Float instead
PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.
Also fixed bug with unclosed paths causing discontinuity at the
start/end point.
Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e
M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt
ap...@google.com
PointF was the main (possibly only) mutability issue, marking this as fixed
ch...@google.com
ap...@google.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.graphics:graphics-shapes:1.0.0-alpha04
na...@google.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.compose.compiler:compiler-hosted:1.5.0
ae...@google.com
I temporarily removed the first performance optimization aosp/2636327 in aosp/2676415 because it caused a bug with subcomposition
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 3118e88c6485cb8003b30e6bc09ee302ca4557ce
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Thu Jul 27 16:30:42 2023
Fix and re-enable providers optimization
Disabled the single provider optimization because it could cause
the composition local scope to be incorrect.
It was incorrect when the provider cache was invalidated by a
provider closing just before the composer scope was read again
which causes it to re-lookup the current provider scope. The
`startProvider` call neglected to set the `writerHasAProvider` that
would cause the writer to be inspected for a provider not just the
reader (as the provider is being added during the composition).
The fix is to add this flag at the same location it is in
`startProviders()`.
Bug: 292224893
Fixes: 288169379
Test: ./gradlew :compose:r:r:tDUT
Change-Id: I883c3ba691e41821944a3020eaa1089130cf11a3
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/CompositionLocal.kt
M compose/runtime/runtime/src/nonEmulatorCommonTest/kotlin/androidx/compose/runtime/CompositionLocalTests.kt
https://android-review.googlesource.com/2680337
Branch: androidx-main
commit 3118e88c6485cb8003b30e6bc09ee302ca4557ce
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Thu Jul 27 16:30:42 2023
Fix and re-enable providers optimization
Disabled the single provider optimization because it could cause
the composition local scope to be incorrect.
It was incorrect when the provider cache was invalidated by a
provider closing just before the composer scope was read again
which causes it to re-lookup the current provider scope. The
`startProvider` call neglected to set the `writerHasAProvider` that
would cause the writer to be inspected for a provider not just the
reader (as the provider is being added during the composition).
The fix is to add this flag at the same location it is in
`startProviders()`.
Bug: 292224893
Fixes: 288169379
Test: ./gradlew :compose:r:r:tDUT
Change-Id: I883c3ba691e41821944a3020eaa1089130cf11a3
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/CompositionLocal.kt
M compose/runtime/runtime/src/nonEmulatorCommonTest/kotlin/androidx/compose/runtime/CompositionLocalTests.kt
na...@google.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.compose.runtime:runtime:1.6.0-alpha03androidx.compose.runtime:runtime-android:1.6.0-alpha03androidx.core:core-testing:1.12.0-rc01
Description
Summary
The current API for providing composition locals is very allocation heavy and requires several groups. This can be improved in several ways,
The current API will allocated an array to store the providers and a mall object to for each "provides" parameter. It will then create a persistent map of the provided values and then union of the existing map and this map as the persistent scope.
Some composable function just provide one provides which makes these allocation particularly heavey weight. Also, constructing the union persisent map can be expensive.
Proposals
1. Special case single provider
The single provider can be special cased to avoid the creating of the array, the
ProvidesValueinstance and the persistent map.This would require new API that takes the composition local definition and the value (as well as a corresponding lint with a quick-fix to migrate code to the new API).
2. Allow building the map directly outside composition.
Instead of building the map in composition, the local map (for large maps) can be constructed as helper or outside composition entirely. This is specifically helpful when the map is conditionally built the current API requires paying the local provider overhead multiple times for shared or conditional providers.
This will require a new API that allows providing a composition local map directly.
3. Faster persistent maps
The persistent map is a general map that handles many cases well but it doesn't handle small changes well (such as adding a single value). We could provide a wrapper around the persistent map that will handles the special cases found in the Compose library better such as a singleton wrapper for cases, such when a single value is provided.
This doesn't require any API changes.