Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 8, 2025

User description

Review after #16402

💥 What does this PR do?

As soon as we received remote message just deserialize it. It also improves memory consumption because of unnecessary
JsonElement object.

Before:

| Method                         | Mean       | Error     | StdDev    | Gen0    | Gen1   | Allocated  |
|------------------------------- |-----------:|----------:|----------:|--------:|-------:|-----------:|
| SeleniumLocateNodesAsync       |   2.770 ms | 0.0507 ms | 0.0335 ms | 19.5313 | 3.9063 |  131.12 KB |
| SeleniumCaptureScreenshotAsync | 118.490 ms | 3.2250 ms | 2.1331 ms |       - |      - | 1837.29 KB |

After:

| Method                         | Mean       | Error     | StdDev    | Gen0    | Allocated  |
|------------------------------- |-----------:|----------:|----------:|--------:|-----------:|
| SeleniumLocateNodesAsync       |   2.699 ms | 0.0435 ms | 0.0288 ms | 11.7188 |   87.83 KB |
| SeleniumCaptureScreenshotAsync | 116.053 ms | 2.1708 ms | 1.4358 ms |       - | 1173.63 KB |

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Optimize BiDi message deserialization for better performance

  • Replace deferred JsonElement with immediate type-specific deserialization

  • Reduce memory allocation by ~33% in node location operations

  • Improve screenshot capture performance by ~2ms


Diagram Walkthrough

flowchart LR
  A["Received Message"] --> B["Immediate Deserialization"]
  B --> C["Type-specific JsonTypeInfo"]
  C --> D["Reduced Memory Usage"]
  B --> E["Faster Processing"]
Loading

File Walkthrough

Relevant files
Enhancement
Broker.cs
Core broker deserialization optimization                                 

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Replace generic JsonSerializerContext with specific JsonTypeInfo for
    events
  • Modify CommandInfo to use typed TaskCompletionSource instead of
    JsonElement
  • Update message processing to deserialize immediately using specific
    type info
  • Remove unnecessary JsonElement intermediate storage
+16/-16 
BrowsingContextModule.cs
BrowsingContext event subscription updates                             

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs

  • Update all event subscription calls to use specific JsonTypeInfo
  • Replace generic JsonContext with type-specific context properties
  • Apply changes to navigation, download, context, and user prompt events
+28/-28 
NetworkModule.cs
Network event subscription updates                                             

dotnet/src/webdriver/BiDi/Network/NetworkModule.cs

  • Update network event subscriptions to use specific JsonTypeInfo
  • Apply changes to request, response, fetch error, and auth events
+10/-10 
ScriptModule.cs
Script event subscription updates                                               

dotnet/src/webdriver/BiDi/Script/ScriptModule.cs

  • Update script event subscriptions to use specific JsonTypeInfo
  • Apply changes to message and realm events
+6/-6     
LogModule.cs
Log event subscription updates                                                     

dotnet/src/webdriver/BiDi/Log/LogModule.cs

  • Update log entry event subscriptions to use specific JsonTypeInfo
+2/-2     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Oct 8, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 8, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Refactor CommandInfo for better type safety

Refactor CommandInfo into an abstract base class and a generic derived class.
This will allow storing various command types in the _pendingCommands dictionary
while maintaining type safety.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [348-356]

-class CommandInfo<TResult>(long id, TaskCompletionSource<TResult> taskCompletionSource, JsonTypeInfo jsonResultTypeInfo)
-    where TResult : EmptyResult
+abstract class CommandInfo(long id, JsonTypeInfo jsonResultTypeInfo)
 {
     public long Id { get; } = id;
 
-    public TaskCompletionSource<TResult> TaskCompletionSource { get; } = taskCompletionSource;
+    public JsonTypeInfo JsonResultTypeInfo { get; } = jsonResultTypeInfo;
 
-    public JsonTypeInfo JsonResultTypeInfo { get; } = jsonResultTypeInfo;
-};
+    public abstract void SetResult(object result);
+}
 
+class CommandInfo<TResult> : CommandInfo
+    where TResult : EmptyResult
+{
+    public TaskCompletionSource<TResult> TaskCompletionSource { get; }
+
+    public CommandInfo(long id, TaskCompletionSource<TResult> taskCompletionSource, JsonTypeInfo jsonResultTypeInfo)
+        : base(id, jsonResultTypeInfo)
+    {
+        TaskCompletionSource = taskCompletionSource;
+    }
+
+    public override void SetResult(object result)
+    {
+        TaskCompletionSource.SetResult((TResult)result);
+    }
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion provides a complete and robust solution to a significant type-safety flaw in the PR by refactoring CommandInfo into a base and generic derived class, which is an excellent design pattern for this scenario.

High
Use a non-generic base class

Modify _pendingCommands to use a non-generic base CommandInfo class. This will
allow storing different command result types in the dictionary, improving type
safety.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [40]

-private readonly ConcurrentDictionary<long, CommandInfo<EmptyResult>> _pendingCommands = new();
+private readonly ConcurrentDictionary<long, CommandInfo> _pendingCommands = new();
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a type safety issue where _pendingCommands is constrained to CommandInfo<EmptyResult>, which forces an unsafe cast later. Proposing a non-generic base class is a valid and robust design pattern to solve this problem.

Medium
Learned
best practice
Add null/empty guards for inputs

Guard against null eventName and jsonTypeInfo before using them to avoid
ArgumentNullException later in dictionary access and deserialization.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [159-162]

 public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo jsonTypeInfo)
     where TEventArgs : EventArgs
 {
+    if (string.IsNullOrWhiteSpace(eventName)) throw new ArgumentException("eventName is required.", nameof(eventName));
+    if (jsonTypeInfo is null) throw new ArgumentNullException(nameof(jsonTypeInfo));
     _eventTypesMap[eventName] = jsonTypeInfo;
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to prevent null dereferences and logic errors.

Low
  • Update

@nvborisenko
Copy link
Member Author

nvborisenko commented Oct 8, 2025

Finally, no Gen2 and Gen1 allocations.


private readonly ConcurrentDictionary<long, CommandInfo> _pendingCommands = new();
private readonly BlockingCollection<MessageEvent> _pendingEvents = [];
private readonly ConcurrentDictionary<long, CommandInfo<EmptyResult>> _pendingCommands = new();
Copy link
Contributor

@RenderMichael RenderMichael Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommandInfo<T> always seems to be CommandInfo<EmptyResult>. It does not need to be generic.

class CommandInfo(long id, TaskCompletionSource<EmptyResult> taskCompletionSource, JsonTypeInfo jsonResultTypeInfo)
{
    public long Id { get; } = id;

    public TaskCompletionSource<EmptyResult> TaskCompletionSource { get; } = taskCompletionSource;

    public JsonTypeInfo JsonResultTypeInfo { get; } = jsonResultTypeInfo;
}


public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; } = contexts;

public abstract ValueTask InvokeAsync(object args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this slightly more strongly-typed.

Suggested change
public abstract ValueTask InvokeAsync(object args);
public abstract ValueTask InvokeAsync(EventArgs args);

Copy link
Contributor

@RenderMichael RenderMichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two tiny comments, in general I really like this change! One more leap towards great performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants