Skip to content

Reduce memory allocations in Listen happy path#408

Merged
GregHolmes merged 2 commits into
deepgram:mainfrom
richardszalay:fix/logging-strings
May 29, 2026
Merged

Reduce memory allocations in Listen happy path#408
GregHolmes merged 2 commits into
deepgram:mainfrom
richardszalay:fix/logging-strings

Conversation

@richardszalay
Copy link
Copy Markdown
Contributor

@richardszalay richardszalay commented May 21, 2026

Proposed changes

The library's internal logging facade was not designed to defer string interpolation, so strings are often interpolated by the caller even if that log level is not enabled. In a long-lived server environment, especially when Listen/WebSockets are in use, this leads to unnecessary pressure on the garbage collector.

This PR wraps the offending calls to Log in an IsEnabled condition so that they don't run unless necessary.

I've limited the changes to the Listen "happy path" since that will involve a lot of messages back and forth, but I've left the exception blocks alone.

Types of changes

What types of changes does your code introduce to the community .NET SDK?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update or tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have lint'ed all of my code using repo standards
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@GregHolmes
Copy link
Copy Markdown
Collaborator

Thank you for your contributions Richard, I've reviewed this PR. The guards in AbstractWebSocketClient and Listen/v2 correctly target the biggest allocations: the per-message raw response and per-event Invoking X. event: {response} logs, where each response record's ToString() triggers a full JsonSerializer.Serialize.

Two things I believe we need to address before merging:

  1. Would it be possible for you to please extend coverage to the v1 Listen client. Clients/Listen/v1/WebSocket/Client.cs has the identical hot-path pattern in ProcessDataReceived (lines 655, 659, and the per-event Debug logs at 685/703/721/739/757/775). It's still in active use via the deprecated Live namespace and needs the same guards. While you're there, please also guard ProcessAutoFlush at line 543 (v1) and line 423 (v2), same $"AutoFlush delta..." interpolation pattern, fires every autoflush iteration.

  2. Please drop the inner guards in Log.cs. Adding if (!IsEnabled(...)) return; inside each public log method (Verbose/Debug/…) doesn't actually save the big allocations. By the time Log.Verbose("id", $"...") is called, the caller has already interpolated trace, and Serilog's own Verbose(string) already short-circuits on level. With the new call-site IsEnabled guards in place, the inner guards just add a redundant check per call. The new public Log.IsEnabled(LogLevel) helper is the bit we want to keep.

What do you think?

@richardszalay
Copy link
Copy Markdown
Contributor Author

  1. Yes, happy to apply the fixes there too.
  2. I actually disagree on this one. There are still a large number of hot-path calls that look like this:
Log.Verbose("ListenWSClient.ProcessTextMessage", "ENTER");

Inside the Log methods, those two strings are interpolated into $"{identifier}: {trace}" and I can see that Serilog itself does not have any FormattableString overloads that would avoid the interpolation from happening when the log level is not enabled. The number of these "static" log calls in the hot path mean that the number of unnecessary allocations would still be considerable, and it would still cause more work for the GC to do.

Having said that, I can remove them if you insist because I'd like to get the larger (entire JSON message) allocations removed.

@GregHolmes
Copy link
Copy Markdown
Collaborator

Good point on the inner guards, you're right that the $"{identifier}: {trace}" concat inside Log.Verbose happens before Serilog's IsEnabled check kicks in, so they genuinely save that allocation for every static-string ENTER / LEAVE call in the hot path. Please keep them as-is.

The v1 commit looks great, symmetric coverage with v2. Approving. Thanks for contributing to the SDK. I hope to have a new release for your two PRs in the next day or two.

@GregHolmes GregHolmes merged commit 835644e into deepgram:main May 29, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants