[dotnet] [bidi] Provide type info immediately when serializing

[nvborisenko]

User description

Big performance improvement. This is a continuation of #16392 - so merge that before it.

💥 What does this PR do?

Each module knows what it sends and what it expects to get, just be more precise.

---

PR Type

Enhancement

---

Description

• Refactor BiDi module system for better performance

• Replace generic type parameters with explicit type info

• Centralize JSON serialization configuration

• Implement module factory pattern with shared context

---

Diagram Walkthrough

flowchart LR
  A["BiDi Class"] --> B["Module Factory"]
  B --> C["Shared JSON Context"]
  C --> D["Type-Safe Serialization"]
  E["Broker"] --> F["Direct Type Info"]
  F --> G["Performance Improvement"]

Loading

File Walkthrough

Relevant files

Enhancement

13 files BiDi.cs Refactor module management with factory pattern                    +46/-129 BrowserModule.cs Update command execution with type info                                    +11/-10  BrowsingContextModule.cs Update command execution with type info                                    +41/-41  Broker.cs Replace generic types with explicit serialization                +21/-64  EmulationModule.cs Update command execution with type info                                    +10/-11  InputModule.cs Update command execution with type info                                    +4/-4      LogModule.cs Update command execution with type info                                    +3/-3      Module.cs Implement module factory with shared context                          +22/-2    NetworkModule.cs Update command execution with type info                                    +25/-25  ScriptModule.cs Update command execution with type info                                    +12/-12  SessionModule.cs Update command execution with type info                                    +6/-6      StorageModule.cs Update command execution with type info                                    +4/-4      WebExtensionModule.cs Update command execution with type info                                    +3/-3

---

[qodo-merge-pro]

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]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use POCO for JSON message deserialization Refactor the ProcessReceivedMessage method to deserialize incoming JSON messages into a dedicated POCO class instead of using manual parsing with Utf8JsonReader. dotnet/src/webdriver/BiDi/Communication/Broker.cs [243-297] private void ProcessReceivedMessage(byte[]? data) { if (data is null) return; - long? id = null; - string? type = null; - string? method = null; - Utf8JsonReader resultReader = default; - Utf8JsonReader paramsReader = default; - string? error = null; - string? message = null; - string? stacktrace = null; + var messageData = JsonSerializer.Deserialize<JsonRpcMessage>(data, _jsonOptions)!; - var reader = new Utf8JsonReader(data); - reader.Read(); // "{" + switch (messageData.Type) + { + case "success": + if (messageData.Id is null) throw new JsonException("The remote end responded with 'success' message type, but missed required 'id' property."); - while (reader.TokenType == JsonTokenType.PropertyName) - { - string? propertyName = reader.GetString(); - reader.Read(); + if (_pendingCommands.TryGetValue(messageData.Id.Value, out var successCommand)) + { + successCommand.TaskCompletionSource.SetResult(messageData.Result); + _pendingCommands.TryRemove(messageData.Id.Value, out _); + } + else + { + throw new BiDiException($"The remote end responded with 'success' message type, but no pending command with id {messageData.Id} was found."); + } - switch (propertyName) - { - case "id": - id = reader.GetInt64(); - break; + break; - case "type": - type = reader.GetString(); - break; + case "event": + if (messageData.Method is null) throw new JsonException("The remote end responded with 'event' message type, but missed required 'method' property."); - case "method": - method = reader.GetString(); - break; + if (_eventTypesMap.TryGetValue(messageData.Method, out var eventInfo)) + { + var eventArgs = (EventArgs)messageData.Params.Deserialize(eventInfo.EventType, eventInfo.JsonContext)!; - case "result": - resultReader = reader; // snapshot - break; + var messageEvent = new MessageEvent(messageData.Method, eventArgs); + _pendingEvents.Add(messageEvent); + } + else + { + _logger.LogWarn($"The remote end sent an event '{messageData.Method}', but no event handler was found."); + } - case "params": - paramsReader = reader; // snapshot - break; + break; - case "error": - error = reader.GetString(); - break; + case "error": + if (messageData.Id is null) throw new JsonException("The remote end responded with 'error' message type, but missed required 'id' property."); - case "message": - message = reader.GetString(); - break; + if (_pendingCommands.TryGetValue(messageData.Id.Value, out var errorCommand)) + { + var exception = new BiDiException(messageData.Message ?? "An error occurred processing the command") + { + Error = messageData.Error, + Stacktrace = messageData.Stacktrace + }; + errorCommand.TaskCompletionSource.SetException(exception); + _pendingCommands.TryRemove(messageData.Id.Value, out _); + } + else + { + throw new BiDiException($"The remote end responded with 'error' message type, but no pending command with id {messageData.Id} was found."); + } - case "stacktrace": - stacktrace = reader.GetString(); - break; + break; - default: - reader.Skip(); - break; - } + default: + throw new BiDiException($"The remote end responded with an unknown message type: {messageData.Type}"); + } + } - reader.Read(); - } -... + private class JsonRpcMessage + { + public long? Id { get; set; } + public string? Type { get; set; } + public string? Method { get; set; } + public JsonElement Result { get; set; } + public JsonElement Params { get; set; } + public string? Error { get; set; } + public string? Message { get; set; } + public string? Stacktrace { get; set; } + } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion proposes a good refactoring to improve code clarity and robustness by replacing manual JSON parsing with deserialization into a POCO, which simplifies the ProcessReceivedMessage method.	Medium
	Use provided JSON options for deserialization Store the jsonOptions parameter in the Broker constructor in a private field for future use, as it is currently unused. dotnet/src/webdriver/BiDi/Communication/Broker.cs [55-59] + private readonly JsonSerializerOptions _jsonOptions; + internal Broker(BiDi bidi, Uri url, JsonSerializerOptions jsonOptions) { _bidi = bidi; _transport = new WebSocketTransport(url); + _jsonOptions = jsonOptions; } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the jsonOptions parameter in the Broker constructor is unused, which is a code smell and likely a remnant from refactoring.	Low
Learned best practice	Use concurrent dictionary for events Replace the non-thread-safe dictionary with a concurrent dictionary or protect access with a lock to prevent races when multiple subscriptions occur concurrently. dotnet/src/webdriver/BiDi/Communication/Broker.cs [43-216] -private readonly Dictionary<string, (Type EventType, JsonSerializerContext JsonContext)> _eventTypesMap = []; +private readonly ConcurrentDictionary<string, (Type EventType, JsonSerializerContext JsonContext)> _eventTypesMap = new(); ... public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonSerializerContext jsonContext) where TEventArgs : EventArgs { _eventTypesMap[eventName] = (typeof(TEventArgs), jsonContext); - var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []); + var handlers = _eventHandlers.GetOrAdd(eventName, _ => new List<EventHandler>()); ... } public async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonSerializerContext jsonContext) where TEventArgs : EventArgs { _eventTypesMap[eventName] = (typeof(TEventArgs), jsonContext); - var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []); + var handlers = _eventHandlers.GetOrAdd(eventName, _ => new List<EventHandler>()); ... } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Make concurrency and lifecycle code robust by using concurrent collections safely and avoiding race conditions when registering handlers and types.	Low
	Dispose tasks and cancel loops Ensure DisposeAsync cancels the receive loop, completes the blocking collection, awaits background tasks, and disposes the transport and CTS to avoid leaks. dotnet/src/webdriver/BiDi/Communication/Broker.cs [51-241] -private Task? _receivingMessageTask; -private Task? _eventEmitterTask; -private CancellationTokenSource? _receiveMessagesCancellationTokenSource; -... public async ValueTask DisposeAsync() { - ... (clipped 13 lines) + try + { + _receiveMessagesCancellationTokenSource?.Cancel(); + _pendingEvents.CompleteAdding(); + if (_receivingMessageTask is not null) + { + try { await _receivingMessageTask.ConfigureAwait(false); } catch { /* swallow on dispose */ } + } + if (_eventEmitterTask is not null) + { + try { await _eventEmitterTask.ConfigureAwait(false); } catch { /* swallow on dispose */ } + } + } + finally + { + _receiveMessagesCancellationTokenSource?.Dispose(); + await _transport.DisposeAsync().ConfigureAwait(false); + } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Enforce deterministic and safe resource handling by ensuring all background tasks are canceled and collections completed during disposal.	Low
Update

[nvborisenko]

Reopen this PR, performance is unexpected.
UPD: seems good, rebuild locally:

| Method                         | Mean     | Error   | StdDev  | Allocated |
|------------------------------- |---------:|--------:|--------:|----------:|
| SeleniumCaptureScreenshotAsync | 116.0 ms | 2.69 ms | 1.78 ms |   1.79 MB |

It means very good!