Closed rafael-assis closed 6 months ago
Thank you @rafael-assis for the rigorous investigation, detailed description, and targeted fix 💪 We are excited to integrate this fix internally!
Great work here, much appreciated!
Thank you for building such a useful library, @nalexn ! We are happy to contribute.
Great work here, much appreciated!
We appreciate your collaboration @nalexn! Happy new year! 🎆
Hi @nalexn. We found the issue described below as we continued working on profiling performance bottlenecks in
ViewInspector
and their effects in our test codebase at Airbnb.It's a simple fix and we'd love to get your thoughts on it. Thank you so much again for collaborating with us! 🙏
The problem:
Content.Medium.appending()
funcs are called an excessive number of timesAfter #273 was merged, we noticed that we had a few tests that were still taking a long time to run. We profiled and traced those tests in Instruments again and noticed that the methods that add
environmentModifiers
andenvironmentObjects
to the medium (and their callers) were responsible for most of the running time of those tests.Content.Medium.appending(environmentModifier:)
Content.Medium.appending(environmentObject:)
We then tracked the number of
environmentModifiers
andenvironmentObjects
contained in themedium
property of the InspectableView's content, and noticed a disproportional number of items in those arrays compared to the number of custom modifiers applied to our test view.As an example, consider the following test:
❌ The assertion on the test failed as the
environmentModifiers.count
that should be 15 turns out to be 32767.In further experiments, we concluded that the Big O notation that represents the complexity of the test increased from the expected linear time O(n) to O(2n). (where n is the number of custom environment modifiers applied to the
Text("str")
View.We noticed that the issue does not happen when the
.environment()
modifier call is applied directly to the view:✅ The test above passes.
The fix: Making custom modifier logic linear as the unwrapping traversal
The cause of the issue is that the code that handles the unwrapping of custom modifiers in
func unwrappedModifiedContent()
duplicates all the properties that were previously passed by the unwrapping of its container View.The duplication happens because the items in the medium that were originally used to extract the current modifier content view are also returned in the
InspectableView
's Content.Medium by the find call.The same items will then be appended again to the original medium that will be passed as a parameter to the recursive call that unwraps the modifier's child content.
The fix consists of simply recreating the medium with the 3 properties (
transitiveModifiers
,environmentModifiers
andenvironmentObject
) reset to the values of theContent.Medium
in theInspectableView
(viewModifierContent
variable) returned by the find call.Results in tests for our production code
We could see dramatic improvements in running time of the tests that flagged this issue in our CI environment:
Testing
We are adding 6 test cases to validate the application of custom modifiers and SwiftUI built-in modifiers in scenarios where the 3 properties of the
Content.Medium
struct are affected by this issue.Content.Medium
propertytransitiveModifiers
testMultipleTransitiveModifiers
testMultipleCustomTransitiveModifiers
environmentModifiers
testMultipleEnvironmentModifiers
testMultipleCustomEnvironmentModifiers
environmentObjects
testMultipleEnvironmentObjects
testMultipleCustomEnvironmentObjectModifiers
The tests to validate the direct application of the built-in SwiftUI modifiers were added only for comparison purposes. They show the correct behavior and are used as a reference for the correct results of the custom modifier application scenarios that were fixed.
Please review:
@nalexn @bachand