Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/string builder optimise #68

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

graemefoster
Copy link

@graemefoster graemefoster commented Apr 3, 2023

Alternate approaches for buffering data before sending to Splunk, aiming to avoid Large Object Heap allocations. I've got 3 alternative approaches each which allocates less on the LOH than the original...

Keen for feedback on if we should offer them all / remove some..

Here's some memory profiles

Original
image

StringBuilder ToString() at time of sending content:
image

Buffering to file, then using FileContent
image

Using a PushStreamContent to stream smaller JSON strings at point of HTTP send:
image

@@ -1,12 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net45</TargetFrameworks>
<TargetFrameworks>net452;net6.0</TargetFrameworks>

Choose a reason for hiding this comment

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

can we add support for net462

Copy link
Author

Choose a reason for hiding this comment

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

Net452 is fine - net 462 project can happily reference the 452 build

serializedEventInfo = SerializeEventInfo(ei);
}
else
if (formatter != null)

Choose a reason for hiding this comment

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

make it possible for DE and NL locales to get data through this call.

Copy link
Author

Choose a reason for hiding this comment

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

I can see you have a PR already to do this. This one should stay focused on improving memory usage, and your PR can provide DE / NL locale support.

Copy link

@jayaraut jayaraut left a comment

Choose a reason for hiding this comment

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

please update as per suggestions

@graemefoster
Copy link
Author

please update as per suggestions

@jayaraut I don't think there's any need for this PR to address those issues - pls see my comments.

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.

2 participants