feat: Optimize OptionCollector by reducing dictionary allocations#1035
Conversation
…alls - Refactored `ParseCapabilitiesIfWindows` and `ParseCapabilitiesIfUNIX` to accept a pre-computed dictionary instead of calling `ToDictionary()` internally. - Updated `Arguments` property getter to call `ToDictionary()` exactly once and reuse the result. - Optimized `AddCapabilities` to merge options in-place, avoiding multiple dictionary allocations and object re-instantiations. - Improved efficiency of option collection by reducing O(N) operations and memory pressure.
…nary-allocation-18318062666670431403
…est for OptionCollector
Dor-bl
requested review from
KazuCocoa and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Refactors how OptionCollector merges Appium capabilities to reduce allocations and ensure the latest merged capabilities are reflected in the Appium server arguments, with an added integration test to validate repeated capability additions.
Changes:
- Refactored
OptionCollector.AddCapabilitiesto merge directly into the internal options instance instead of rebuilding a merged dictionary. - Updated capability parsing helpers to accept a capabilities dictionary parameter and updated
Argumentsto pass a single merged dictionary through. - Added an integration test to verify multiple
AddCapabilitiescalls result in merged capabilities in the generated arguments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/integration/ServerTests/AppiumLocalServerLaunchingTest.cs |
Adds a test asserting multiple AddCapabilities calls yield merged --default-capabilities output. |
src/Appium.Net/Appium/Service/Options/OptionCollector.cs |
Refactors capability merging/parsing to reduce allocations and centralize merged dictionary handling for argument generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public OptionCollector AddCapabilities(AppiumOptions options) | ||
| { | ||
| if (this.options == null) | ||
| { | ||
| this.options = options; | ||
| } | ||
| else | ||
| { | ||
| IDictionary<string, object> originalDictionary = this.options.ToDictionary(); | ||
| IDictionary<string, object> givenDictionary = options.ToDictionary(); | ||
| IDictionary<string, object> result = new Dictionary<string, object>(originalDictionary); | ||
|
|
||
| foreach (var item in givenDictionary) | ||
| { | ||
| if (originalDictionary.ContainsKey(item.Key)) | ||
| { | ||
| result[item.Key] = item.Value; | ||
| } | ||
| else | ||
| { | ||
| result.Add(item.Key, item.Value); | ||
| } | ||
| } | ||
|
|
||
| this.options = new AppiumOptions(); | ||
|
|
||
| foreach (var item in result) | ||
| { | ||
| this.options.AddAdditionalAppiumOption(item.Key, item.Value); | ||
| } |
There was a problem hiding this comment.
AddCapabilities now keeps a reference to the first AppiumOptions instance passed in and, on subsequent calls, merges by calling this.options.AddAdditionalAppiumOption(...). This mutates the caller-provided options object (the first one), whereas the previous implementation replaced this.options with a new AppiumOptions when merging, avoiding side effects. To preserve the prior behavior and prevent unexpected external mutation, consider always copying capabilities into an internally-owned AppiumOptions (e.g., initialize this.options as a new AppiumOptions and merge in options.ToDictionary()), rather than storing and mutating the passed-in instance.
Dor-bl
deleted the
perf-optimize-option-collector-dictionary-allocation-18318062666670431403
branch
List of changes
This pull request refactors how capabilities are merged and passed to the Appium server in the .NET client, improving code clarity and correctness. It also introduces a new integration test to verify that capabilities are merged as expected when added multiple times.
Capabilities merging and argument handling:
AddCapabilitiesmethod inOptionCollectorto directly merge new capabilities into the internaloptionsobject, simplifying the logic and reducing unnecessary dictionary copying.ParseCapabilitiesIfWindowsandParseCapabilitiesIfUNIXto accept a capabilities dictionary as a parameter instead of relying on the internaloptionsobject, making these methods more flexible and testable. [1] [2]Argumentsproperty to use the new capability merging logic and pass the merged capabilities dictionary to the parsing methods, ensuring arguments reflect the latest capabilities state. [1] [2]Testing improvements:
AddCapabilities_MergesCapabilitiesWhenCalledMultipleTimesinAppiumLocalServerLaunchingTest.csto verify that callingAddCapabilitiesmultiple times correctly merges all provided capabilities into the arguments.Minor code cleanup:
using System.Reflection;directive in the test file to support reflection for accessing private properties.Types of changes
What types of changes are you proposing/introducing to the .NET client?
Put an
xin the boxes that applyDocumentation
This can be done by navigating to the documentation section on http://appium.io selecting the appropriate command/endpoint and clicking the 'Edit this doc' link to update the C# example
Integration tests
Details
Please provide more details about changes if necessary. You can provide code samples showing how they work and possible use cases if there are new features. Also, you can create gists with pasted C# code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub