Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide Metal drawables from Kotlin runtime #1390

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

elijah-semyonov
Copy link

@elijah-semyonov elijah-semyonov commented Jun 7, 2024

Explicitly manage lifetime of metal drawables by hiding them from Kotlin runtime.

Fixes a memory spike when quickly resizing metal layer as in a scenario reported in the issue

Otherwise associated drawable pools are retained until next GC, which can happen after inadequate amount of drawables with new sizes are allocated

Before:
Screenshot 2024-06-06 at 13 33 46

After:
Screenshot 2024-06-07 at 10 04 18

Testing

Resize the popup with ComposeUIViewController to different detents inside Demo IosBugs/PopupStretching
Requires either patch:

Index: compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt b/compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt
--- a/compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt	(revision 39d436a845d0cbe9526a6c24933268aacf83b2eb)
+++ b/compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.uikit.kt	(date 1717747754590)
@@ -558,6 +558,7 @@
                 asDpRect().toRect()
             }
         }
+        onComposeSceneInvalidate()
         scene.size = IntSize(
             width = boundsInPx.width.roundToInt(),
             height = boundsInPx.height.roundToInt()

Or #1387

Release Notes

Fixes - iOS

  • Fixed a memory spike when continuously resizing the ComposeUIViewController (such as when used in modal sheet presentation context with different detents)

@elijah-semyonov elijah-semyonov changed the title Hide metal drawables from Kotlin lifetime Hide metal drawables from Kotlin runtime Jun 7, 2024
@elijah-semyonov elijah-semyonov changed the title Hide metal drawables from Kotlin runtime Hide Metal drawables from Kotlin runtime Jun 7, 2024
import platform.UIKit.UISheetPresentationControllerDetent
import platform.UIKit.sheetPresentationController

//val bottomSheetUIViewController =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, removed


- (instancetype)initWithMetalLayer:(CAMetalLayer *)metalLayer;

- (void * CMP_OWNED)nextDrawable;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking about slightly more generic solution.
What do you think about creating analog of "Manual Reference Counter" that can wrap various classes and hide them from Kotlin GC?
Or maybe even play a bit with NSProxy to provide sufficient interface for Kotlin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to model this approach in my head, but didn't succeed. Do you have proof-of-concept available?

@@ -0,0 +1,29 @@
//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check copyright

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment on lines +378 to +379
// scheduleDrawablePresentation consumes metalDrawable
// don't use metalDrawable after this call
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just nullify it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a smart cast val, doing so will require to put force-unwraps everywhere

@elijah-semyonov
Copy link
Author

elijah-semyonov commented Jun 10, 2024

The particular web tests were reported to be flaky and are unrelevant to the PR, merging ignoring the failure.
Screenshot 2024-06-10 at 13 23 11

@elijah-semyonov elijah-semyonov merged commit 0d4294c into jb-main Jun 10, 2024
7 of 8 checks passed
@elijah-semyonov elijah-semyonov deleted the es/move-drawables-to-objc branch June 10, 2024 11:23
@chrisjenx
Copy link

Do you reckon this is related? JetBrains/compose-multiplatform#4852

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants