[Perf improvement] Implementing memoization for sanitizeNamespace and replaceGenericParameters funcs. #273

rafael-assis · 2023-11-14T05:30:56Z

Hi @nalexn. Picking up were we left off on #263, we have identified another opportunity for performance improvements in ViewInspector. @bachand and I discussed it and we'd love to get your thoughts on it.
Thank you so much for your time! 🙏

Context: performance improvements under larger SwiftUI View hierarchies

We have profiled and traced some of our slowest tests in Instruments showed that the 2 funcs defined in the String extension were the top offenders in terms of running time.

String.replacingGenericParameters(_:)
String.sanitizingNamespace()

The combined time of all calls to both of these functions accounted for almost 50% of the entire test running time:

Weight(s)	Weight(%)	Self Weight	Symbol Name
28.26 s	100.0%	0 s	UnitTestHost (16171)
8.08 s	28.5%	8.08 s	String.replacingGenericParameters(_:)
5.80 s	20.5%	5.80 s	String.sanitizingNamespace()

It was already known that these 2 functions are called thousands of times for a single test case in our codebase.
This fact motivated an investigation on how many times these 2 funcs are called for the exact same input in order to inform a potential decision to memoize the calls.

A histogram of the calls to sanitizingNamespace for each input was built from a single feature integration test run.
We found that for some types, the func sanitizingNamespace thousands of times for them. For example:

    - key : "SwiftUI.Environment<Swift.Optional<Swift.AnyHashable>>"
    - value : 3640

    - key : "SwiftUICoreUI.ScrollEventDispatch"
    - value : 144817

    - key : "Swift.Bool"
    - value : 3906

    - key : "SwiftUI.Environment<PrimitiveTypesCoreUI.AdaptiveTraitCollection>"
    - value : 19052
 
    - key : "(PrimitiveTypesCoreUI.InteractivityState) -> SwiftUI.ModifiedContent<SwiftUI._ViewModifier_Content<PrimitiveTypesCoreUI.Interactive<PrimitiveTypesCoreUI.TextStyleViewModifier>>, PrimitiveTypesCoreUI.TextStyleViewModifier>"
    - value : 17912

Changes

It was then clear that caching the output of these expensive string manipulation operations would be worth it.
This is exactly what theses changes do.
It's a simple memoization which stores the computed value to be returned directly instead of recalculating them repeatedly.

Testing

Ran tests at Airbnb that use ViewInspector

Local runs of the the test used to profile ViewInspector's execution time showed a substantial improvement of running time. During the 2 Unit Tests CI runs of the changes in this PR, we noticed an improvement of about 86% (19 seconds). That particular test took ~22 seconds to run before the optimizations and ~3 seconds after the optimizations:

Although the unit tests in ViewInspector are more lightweight compared to the tests we run at Airbnb, we could also see a good improvement of the overall running time of all tests of the library:

Before	After

Please review:

@nalexn
@michael-bachand

…rameters funcs.

rafael-assis · 2023-11-14T05:32:36Z

Sources/ViewInspector/Inspector.swift

+    static func sanitizeNamespace(ofTypeName typeName: String) -> String {
+        var str = typeName
+
+        if let sanitized = sanitizedNamespacesCache[typeName] {


I moved the exact implementation from the String extension to here and added the memoization calls here and in line 26.

rafael-assis · 2023-11-14T05:33:27Z

Sources/ViewInspector/Inspector.swift

+        if balance == 0 {
+            return String(typeName[..<start]) +
+            replacement +
+            replaceGenericParameters(inTypeName: String(typeName[current...]),


We still call into the memoized func to potentially leverage from memoized subtypes.

rafael-assis · 2023-11-14T05:34:29Z

Sources/ViewInspector/Inspector.swift

+    private static var sanitizedNamespacesCache: [String: String] = [:]
+
+    // swiftlint:disable force_try
+    private static let sanitizeNamespaceRegex = try! NSRegularExpression(pattern: "\\.\\(unknown context at ..........\\)")


We thought it'd be better to keep this regex as a constant instead of recreating it every time sanitizeNamespace is called.

rafael-assis · 2023-11-14T05:39:06Z

Sources/ViewInspector/Inspector.swift

    static func guardType(value: Any, namespacedPrefixes: [String], inspectionCall: String) throws {
-        
+


@nalexn do you have a preference regarding the style of tabs / spaces / empty lines?
I can revert those to the original state if you prefer. Let me know.

Nah, I like this more. I've googled how to tweak the Xcode settings to not insert spaces

Sounds good! I like this better too!

@nalexn in case it's helpful, the Airbnb style guide has a script that you can run that enables various editor settings like this one that we find helpful in our large project.

nalexn · 2023-11-14T06:09:21Z

That is a substantial performance improvement, good job! Before merging this in, I suggest an additional slight refactor of sanitizeNamespace in this fasion.
The part where it removes "(extension in ...)" isn't yet confirmed to be helpful, but it'd be best to make sure your final version supports adding more patterns for prefix removal

rafael-assis · 2023-11-14T17:25:17Z

That is a substantial performance improvement, good job! Before merging this in, I suggest an additional slight refactor of sanitizeNamespace in this fasion. The part where it removes "(extension in ...)" isn't yet confirmed to be helpful, but it'd be best to make sure your final version supports adding more patterns for prefix removal

Thank you for the super quick turnaround, @nalexn! 🙏
I have updated sanitizeNamespace to support multiple patterns as you suggested.

I left the new pattern disabled for now with a comment. Let me know what you think of the approach.

rafael-assis · 2023-11-14T17:27:49Z

Sources/ViewInspector/Inspector.swift

+
+    private static let sanitizeNamespaceRegex = {
+        // swiftlint:disable force_try
+        try! NSRegularExpression(pattern: sanitizeNamespacePatterns.joined(separator: "|"))


I believe having multiple patterns joined by an OR operator (|) might be faster than running the stringByReplacingMatches for each of them. Since we're not looking to replace different patterns with different strings, this seemed like the most performant approach.

This makes sense, though I wonder if we want to wrap patterns in ( and ) before OR'ing them together to ensure that each pattern is considered atomically?

I believe the fact that we add an | already treats both sides of it as individual patterns to be matched. But I don't think it's an issue adding them, and it makes it more clear too, so I added them. 👍

rafael-assis · 2023-11-14T17:29:00Z

Sources/ViewInspector/Inspector.swift

+        // This pattern may be helpful to solve issue #268.
+        // It will remain disabled until it is confirmed.
+        // https://github.com/nalexn/ViewInspector/issues/268
+        //"\\(extension in [a-zA-Z0-9]*\\)\\:",


@nalexn I left this disabled since #268 is not yet resolved with a conclusion that this additional pattern helps.
I also left a comment for context.

rafael-assis · 2023-11-14T17:29:39Z

Sources/ViewInspector/Inspector.swift

    static func guardType(value: Any, namespacedPrefixes: [String], inspectionCall: String) throws {
-        
+


Sounds good! I like this better too!

bachand

Great work @rafael-assis and thanks for the quick review @nalexn ! 🙏

bachand · 2023-11-15T02:55:53Z

Sources/ViewInspector/Inspector.swift

+    private static var replacedGenericParametersCache: [String : [String : String]] = [:]
+    private static var sanitizedNamespacesCache: [String: String] = [:]


This strategy for caching is not thread-safe. We think that's OK (i.e. thread safety isn't necessary) but I wanted to flag it to @nalexn out of an abundance of caution 👍

Thanks for mentioning that @bachand!
I missed this comment and did want to flag it to @nalexn as well! 👍

Yeah, no need to solve a problem that may not even exist. I'm 99% sure it should be fine without synchronization, we can add it later if needed

bachand · 2023-11-15T02:57:44Z

Sources/ViewInspector/Inspector.swift

+    /// - Parameter typeName: The raw type name. (e.g. ``SomeTypeName.(unknown context at $138b3290c).SomePropertyName``)
+    /// - Returns: The sanitized type name. (e.g. ``SomeTypeName.SomePropertyName``)


Suggested change

/// - Parameter typeName: The raw type name. (e.g. ``SomeTypeName.(unknown context at $138b3290c).SomePropertyName``)

/// - Returns: The sanitized type name. (e.g. ``SomeTypeName.SomePropertyName``)

/// - Parameter typeName: The raw type name. (e.g. `SomeTypeName.(unknown context at $138b3290c).SomePropertyName`)

/// - Returns: The sanitized type name. (e.g. `SomeTypeName.SomePropertyName`)

Nit: My understanding is that the double backticks are used to create links to other types when documentation comments are displayed in Xcode. Since these are example types, I think single backticks are more appropriate.

Makes sense! I'll update to single backticks! 👍

bachand · 2023-11-15T02:58:16Z

Sources/ViewInspector/Inspector.swift

+    ///   - typeName: The original type name. (e.g. ``SomeTypeName<SomeGenericTypeParameter>``)
+    ///   - replacement: The string to replace the generic parameters with. (e.g. "<EmptyView>")
+    /// - Returns: The type name with its generic parameters replaced. (e.g. ``SomeTypeName<EmptyView>``)


Suggested change

/// - typeName: The original type name. (e.g. ``SomeTypeName<SomeGenericTypeParameter>``)

/// - replacement: The string to replace the generic parameters with. (e.g. "<EmptyView>")

/// - Returns: The type name with its generic parameters replaced. (e.g. ``SomeTypeName<EmptyView>``)

/// - typeName: The original type name. (e.g. `SomeTypeName<SomeGenericTypeParameter>`)

/// - replacement: The string to replace the generic parameters with. (e.g. "<EmptyView>")

/// - Returns: The type name with its generic parameters replaced. (e.g. `SomeTypeName<EmptyView>`)

Nit: See similar comment above for rationale.

bachand · 2023-11-15T03:00:05Z

Sources/ViewInspector/Inspector.swift

+        if balance == 0 {
+            return String(typeName[..<start]) +
+            replacement +
+            replaceGenericParameters(inTypeName: String(typeName[current...]),


bachand · 2023-11-15T03:02:06Z

Sources/ViewInspector/Inspector.swift

+
+    private static let sanitizeNamespaceRegex = {
+        // swiftlint:disable force_try
+        try! NSRegularExpression(pattern: sanitizeNamespacePatterns.joined(separator: "|"))


This makes sense, though I wonder if we want to wrap patterns in ( and ) before OR'ing them together to ensure that each pattern is considered atomically?

bachand · 2023-11-15T03:04:19Z

Sources/ViewInspector/Inspector.swift

    static func guardType(value: Any, namespacedPrefixes: [String], inspectionCall: String) throws {
-        
+


@nalexn in case it's helpful, the Airbnb style guide has a script that you can run that enables various editor settings like this one that we find helpful in our large project.

rafael-assis · 2023-11-20T21:56:46Z

That is a substantial performance improvement, good job! Before merging this in, I suggest an additional slight refactor of sanitizeNamespace in #268 (comment).
The part where it removes "(extension in ...)" isn't yet confirmed to be helpful, but it'd be best to make sure your final version supports adding more patterns for prefix removal

Hi @nalexn. Do you think the changes I made fully addressed your suggestions?
Let me know if you need me to make any further adjustment before you merge this PR.

Thank you!

nalexn · 2023-11-25T08:20:31Z

Merged. Thank you @rafael-assis and @bachand !

Implementing memoization for sanitizingNamespace and replaceGenericPa…

fd5f940

…rameters funcs.

rafael-assis commented Nov 14, 2023

View reviewed changes

sanitizeNamespace: Support for multiple patterns

c129798

rafael-assis commented Nov 14, 2023

View reviewed changes

bachand approved these changes Nov 15, 2023

View reviewed changes

Updating ` with in comments, enclosing patterns between parenthesis.

4f1b3b6

rafael-assis requested a review from nalexn November 16, 2023 19:17

nalexn merged commit 1be47b7 into nalexn:0.9.9 Nov 25, 2023

nalexn added the pending release A fixed issue that'll be released in an upcoming update label Nov 25, 2023

nalexn mentioned this pull request Nov 25, 2023

Inspect fails to find views due to guardType(value:namespacedPrefixes:inspectionCall:) incorrectly failing #268

Open

nalexn removed the pending release A fixed issue that'll be released in an upcoming update label Dec 3, 2023

This was referenced Dec 13, 2023

[Perf improvement] Fixing Content.Medium creation for the unwrapping of custom modifiers. rafael-assis/ViewInspector#1

Draft

[Perf improvement] Fixing Content.Medium creation for the unwrapping of custom modifiers. #284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Perf improvement] Implementing memoization for sanitizeNamespace and replaceGenericParameters funcs. #273

[Perf improvement] Implementing memoization for sanitizeNamespace and replaceGenericParameters funcs. #273

rafael-assis commented Nov 14, 2023

rafael-assis Nov 14, 2023

rafael-assis Nov 14, 2023

bachand Nov 15, 2023

rafael-assis Nov 14, 2023

rafael-assis Nov 14, 2023

nalexn Nov 14, 2023

rafael-assis Nov 14, 2023

bachand Nov 15, 2023

nalexn commented Nov 14, 2023

rafael-assis commented Nov 14, 2023

rafael-assis Nov 14, 2023 •

edited

Loading

bachand Nov 15, 2023

rafael-assis Nov 15, 2023

rafael-assis Nov 14, 2023

rafael-assis Nov 14, 2023

bachand left a comment

bachand Nov 15, 2023

rafael-assis Nov 15, 2023

nalexn Nov 25, 2023

bachand Nov 15, 2023

rafael-assis Nov 15, 2023

bachand Nov 15, 2023

bachand Nov 15, 2023

bachand Nov 15, 2023

bachand Nov 15, 2023

rafael-assis commented Nov 20, 2023

nalexn commented Nov 25, 2023

		static func guardType(value: Any, namespacedPrefixes: [String], inspectionCall: String) throws {

		private static var replacedGenericParametersCache: [String : [String : String]] = [:]
		private static var sanitizedNamespacesCache: [String: String] = [:]

		/// - Parameter typeName: The raw type name. (e.g. ``SomeTypeName.(unknown context at $138b3290c).SomePropertyName``)
		/// - Returns: The sanitized type name. (e.g. ``SomeTypeName.SomePropertyName``)

[Perf improvement] Implementing memoization for sanitizeNamespace and replaceGenericParameters funcs. #273

[Perf improvement] Implementing memoization for sanitizeNamespace and replaceGenericParameters funcs. #273

Conversation

rafael-assis commented Nov 14, 2023

Context: performance improvements under larger SwiftUI View hierarchies

Changes

Testing

Please review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalexn commented Nov 14, 2023

rafael-assis commented Nov 14, 2023

rafael-assis Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafael-assis commented Nov 20, 2023

nalexn commented Nov 25, 2023

rafael-assis Nov 14, 2023 •

edited

Loading