[dotnet] [bidi] Deserialize message fast instead of defer it #16403

nvborisenko · 2025-10-08T17:37:58Z

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"]

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

qodo-merge-pro · 2025-10-08T17:38:32Z

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

qodo-merge-pro · 2025-10-08T17:40:16Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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
Possible issue	✅ ~~Use a non-generic base class~~ Suggestion Impact: The commit replaced ConcurrentDictionary> with ConcurrentDictionary and updated CommandInfo from generic to non-generic, aligning with the suggestion. code diff: - private readonly ConcurrentDictionary<long, CommandInfo<EmptyResult>> _pendingCommands = new(); - private readonly BlockingCollection<MessageEvent> _pendingEvents = []; + private readonly ConcurrentDictionary<long, CommandInfo> _pendingCommands = new(); + private readonly BlockingCollection<(string Method, EventArgs Params)> _pendingEvents = []; private readonly Dictionary<string, JsonTypeInfo> _eventTypesMap = []; private readonly ConcurrentDictionary<string, List<EventHandler>> _eventHandlers = new(); @@ -147,7 +147,7 @@ var timeout = options?.Timeout ?? TimeSpan.FromSeconds(30); using var cts = new CancellationTokenSource(timeout); cts.Token.Register(() => tcs.TrySetCanceled(cts.Token)); - var commandInfo = new CommandInfo<EmptyResult>(command.Id, tcs, jsonResultTypeInfo); + var commandInfo = new CommandInfo(command.Id, tcs, jsonResultTypeInfo); _pendingCommands[command.Id] = commandInfo; 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(); `[Suggestion processed]` 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 · 2025-10-08T18:10:32Z

Finally, no Gen2 and Gen1 allocations.

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

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

RenderMichael

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

nvborisenko added 2 commits October 8, 2025 19:07

[bidi] [dotnet] Use events JsonTypeInfo for deserialization

5e84d9d

Immediate

d7f0493

selenium-ci added the C-dotnet .NET Bindings label Oct 8, 2025

qodo-merge-pro bot added the Review effort 2/5 label Oct 8, 2025

nvborisenko added 2 commits October 8, 2025 20:51

Merge remote-tracking branch 'upstream/trunk' into bidi-deserialize-fast

ea77d15

Event args type safety

ba6b7ed

nvborisenko added 2 commits October 8, 2025 22:22

Remove unused Mesage type

02796ea

Merge branch 'trunk' into bidi-deserialize-fast

ca0572a

nvborisenko requested a review from RenderMichael October 9, 2025 18:34

RenderMichael reviewed Oct 9, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Communication/Broker.cs Outdated Show resolved Hide resolved

RenderMichael reviewed Oct 10, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Communication/EventHandler.cs Outdated Show resolved Hide resolved

RenderMichael approved these changes Oct 10, 2025

View reviewed changes

nvborisenko added 3 commits October 10, 2025 13:56

Non generic CommandInfo

b805e93

Concrete EventArgs

82b1a6b

Merge branch 'trunk' into bidi-deserialize-fast

41a4465

nvborisenko merged commit e6d99a3 into SeleniumHQ:trunk Oct 10, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[dotnet] [bidi] Deserialize message fast instead of defer it #16403

[dotnet] [bidi] Deserialize message fast instead of defer it #16403

nvborisenko commented Oct 8, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Oct 8, 2025 •

edited

Loading

Uh oh!

qodo-merge-pro bot commented Oct 8, 2025 •

edited

Loading

Uh oh!

nvborisenko commented Oct 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

RenderMichael left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

[dotnet] [bidi] Deserialize message fast instead of defer it #16403

[dotnet] [bidi] Deserialize message fast instead of defer it #16403

Conversation

nvborisenko commented Oct 8, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

💥 What does this PR do?

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-merge-pro bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

nvborisenko commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RenderMichael left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nvborisenko commented Oct 8, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Oct 8, 2025 •

edited

Loading

qodo-merge-pro bot commented Oct 8, 2025 •

edited

Loading

nvborisenko commented Oct 8, 2025 •

edited

Loading