green-code-initiative / ecoCode-challenge

Emboard in the hackhatons serie for improving ecoCode
3 stars 4 forks source link

[Hackathon 2024][Niobium][iOS] ForEach filtering should be done before the ForEach block instead of inside the ForEach block in SwiftUI #108

Open ThibaultFarnier opened 4 months ago

ThibaultFarnier commented 4 months ago

Rule title

Avoid usage of if for filtering purposes inside ForEach.

Language and platform

Swift 5.0+, iOS 13+

Rule description

In SwiftUI, when using ForEach blocks, ifs should be avoided to pick the displayed view.

In order for SwiftUI to have best performances, it needs to know at compile-time the "tree" of Views it is going to display.

Using an if prevents SwiftUI from knowing what View will be picked at runtime, and this can lead to performance issues, especially when displaying lists of elements, even when lazily loaded inside a List.

If inside a ForEach block an if is used to selectively display a View, then the filtering operation should be performed before the ForEach block instead of inside.

Bad code:

import SwiftUI

struct CellContent {
    let isEmpty: Bool

    static func generateCells() -> [CellContent] {
        (0..<10_000).map { _ in CellContent(isEmpty: .random()) }
    }
}

struct ContentView: View {
    let cells = CellContent.generateCells()

    var body: some View {
        List {
            ForEach(Array(cells.enumerated()), id: \.0) {
                if !$0.element.isEmpty {
                    RandomColorView()
                }
            }
        }
    }
}

struct RandomColorView: View {
    var body: some View {
        Color(uiColor: .random())
    }
}

extension UIColor {
    static func random() -> UIColor {
        [.yellow, .red, .blue, .black , .brown, .cyan, .green].randomElement()!
    }
}

Good code:

import SwiftUI

struct CellContent {
    let isEmpty: Bool

    static func generateCells() -> [CellContent] {
        (0..<10_000).map { _ in CellContent(isEmpty: .random()) }
    }
}

struct ContentView: View {
    let cells = CellContent.generateCells()

    var body: some View {
        List {
            ForEach(Array(cells.filter { !$0.isEmpty }.enumerated()), id: \.0) { _ in
                RandomColorView()
            }
        }
    }
}

struct RandomColorView: View {
    var body: some View {
        Color(uiColor: .random())
    }
}

extension UIColor {
    static func random() -> UIColor {
        [.yellow, .red, .blue, .black , .brown, .cyan, .green].randomElement()!
    }
}

Rule short description

When performing filtering operations to selectively display a single View inside a ForEach block in SwiftUI, filtering should be performed before the ForEach block, not inside of it.

Rule justification

Assuming the following existing code:

import SwiftUI

struct CellContent {
    let isEmpty: Bool

    static func generateCells() -> [CellContent] {
        (0..<10_000).map { _ in CellContent(isEmpty: .random()) }
    }
}

struct ContentView: View {
    let cells = CellContent.generateCells()

    var body: some View {
        List {
            ForEach(Array(cells.enumerated()), id: \.0) {
                if !$0.element.isEmpty {
                    RandomColorView()
                }
            }
        }
    }
}

struct RandomColorView: View {
    var body: some View {
        Color(uiColor: .random())
    }
}

extension UIColor {
    static func random() -> UIColor {
        [.yellow, .red, .blue, .black , .brown, .cyan, .green].randomElement()!
    }
}

This code is problematic because SwiftUI doesn't know at compile-time if it will display RandomColorView() inside the ForEach block or an EmptyView, and can trigger performance issues especially when displaying numerous items inside of a List / ScrollView.

As we can see in this video (performed on a real device iPhone 12 Pro):

https://github.com/green-code-initiative/ecoCode-challenge/assets/5621515/0e67ebb1-da0f-4bf6-b0da-b84185675c86

It takes roughly thirty seconds to scroll through the whole list.

This video was analyzed in parallel with Instruments to check CPU performances, here are the results below:

Screenshot 2024-05-30 at 10 36 20

We can see that often the CPU is heavily used and hangs several times.

If instead we perform a filtering operation before the ForEach block, then SwiftUI knows at compile-time that it will always display a RandomColorView and knows how to optimize what's displayed inside the list.

struct CellContent {
    let isEmpty: Bool

    static func generateCells() -> [CellContent] {
        (0..<10_000).map { _ in CellContent(isEmpty: .random()) }
    }
}

struct ContentView: View {
    let cells = CellContent.generateCells()

    var body: some View {
        List {
            ForEach(Array(cells.filter { !$0.isEmpty }.enumerated()), id: \.0) { _ in
                RandomColorView()
            }
        }
    }

    @ViewBuilder
    func cellView() -> some View {
        RandomColorView()
    }
}

We can see in the video below (same iPhone 12 Pro):

https://github.com/green-code-initiative/ecoCode-challenge/assets/5621515/a6319c25-f0cd-4929-ae7d-e03f70707bde

that performances are better, as it takes only 10 seconds to reach the bottom of the 10k elements list.

This is confirmed by Instruments as well:

Screenshot 2024-05-30 at 10 38 56

as there are no hangs, and although CPU is under heavy use, it is for a way shorter amount of time.

Similar investigation results can be found in this article: https://martinmitrevski.com/2024/01/02/anyviews-impact-on-swiftui-performance/

Instruments trace file: ForEachIf.trace.zip

Severity / Remediation Cost

Severity: Major (depends of on the size of the list)

Remediation: Medium

Remediation consists in performing the filtering operation before the ForEach block.

Implementation principle

In a ForEach loop, check if an if is used to selectively display a single View OR if it is used to choose between a single View OR EmptyView.

Valid triggering examples:

ForEach(/*item*/) { item in
    if condition {
        AView()
    }
}
ForEach(/*item*/) { item in
    if condition {
        AView()
    } else {
        EmptyView()
    }
}

Invalid examples (no easy remediation [apart from switching to UI[Table|Collection]View? 😄] ):

ForEach(/*item*/) { item in
    if condition {
        AView()
    } else {
        BView()
    }
}