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

Memory churn when collecting CStateFlow of List due to === test. #208

Closed
darronschall opened this issue Oct 17, 2022 · 0 comments · Fixed by #209
Closed

Memory churn when collecting CStateFlow of List due to === test. #208

darronschall opened this issue Oct 17, 2022 · 0 comments · Fixed by #209
Assignees
Labels
bug Something isn't working
Milestone

Comments

@darronschall
Copy link
Contributor

darronschall commented Oct 17, 2022

I have a viewModel that exposes a CStateFlow of List that I'm using in SwiftUI. It looks like this:

private val _items = MutableStateFlow<List<Item>>(listOf()).cMutableStateFlow()
var items: CStateFlow<List<Item>> = _items.cStateFlow()

In my SwiftUI view, I use the state helper to subscribe to the stateFlow:

struct ExampleView: View {
  @ObservableObject viewModel = ExampleViewModel()

  var body: some View {
    List(viewModel.state(\.items)) { (item: Item) in
      Text("Item: \(item.id)")
    }
  }
}

This setup is enough to trigger memory churn. Even though items never changes in the viewModel in this example, there's extremely high CPU usage and memory allocations for what should be an idle view.

I did some digging and have a general idea of the root cause. From what I can tell, asking for stateFlow.value inside the state method allocates a new array, every time. I don't know why this happens, perhaps it has something to do with the Kotlin/Swift(ObjC) bridge. You can see this for yourself by adding some simple logging inside of the method:

var value1 = stateFlow.value
var value2 = stateFlow.value
var value3 = stateFlow.value
            
print("value1 = \(value1)")
print("value2 = \(value2)")
print("value3 = \(value3)")
print("stateFlow.value = \(stateFlow.value)")
print("stateFlow.value = \(stateFlow.value)")
print("same instances = \(stateFlow.value === stateFlow.value)")

This will output, in an infinite loop, something like this:

value1 = Optional(<SharedKListAsNSArray 0x600001987900>(

)
)
value2 = Optional(<SharedKListAsNSArray 0x6000019be500>(

)
)
value3 = Optional(<SharedKListAsNSArray 0x6000019aba80>(

)
)
stateFlow.value = Optional(<SharedKListAsNSArray 0x6000019abcc0>(

)
)
stateFlow.value = Optional(<SharedKListAsNSArray 0x6000019f76c0>(

)
)
same instances = false

The solution is to change the equality test of NSArray state helper at https://github.com/icerockdev/moko-mvvm/blob/develop/mvvm-flow/apple/xcode/mokoMvvmFlowSwiftUI/ViewModelState.swift#L86.

Rather than equals: { $0 === $1 }, it should use isEqual(to:) to compare array contents, equals: { $0!.isEqual(to: $1 as! Array<T>) },

After making this change locally, the memory churn goes away.

darronschall added a commit to darronschall/moko-mvvm that referenced this issue Oct 17, 2022
Accessing `stateFlow.value` is not guaranteed to always return the same instance even when the underlying value is unchanged. This  stateFlow behavior was incorrectly causing infinite `objectWillChange` events due to the equality test always reporting `false` even when the new value was, in fact, equal to the previous.

The fix is to go through [`isEqual(to:)`](https://developer.apple.com/documentation/foundation/nsarray/1411770-isequal) to compare the value _contents_ when determine equality before firing off an `objectWillChange` event.
@Alex009 Alex009 linked a pull request Oct 28, 2022 that will close this issue
Alex009 added a commit that referenced this issue Dec 19, 2022
Prevent memory churn described in #208
@Alex009 Alex009 added this to the 0.15.0 milestone Dec 19, 2022
@Alex009 Alex009 added the bug Something isn't working label Dec 19, 2022
@Alex009 Alex009 closed this as completed Dec 19, 2022
@Alex009 Alex009 mentioned this issue Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants