-
-
Notifications
You must be signed in to change notification settings - Fork 418
Reduce HGlobal allocation in CallResult scenario #674
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
base: master
Are you sure you want to change the base?
Conversation
Changed .NET Standard project to make use of new(.NET 5+) API, Unity will also get benefit from it. Compatibility to Framework and old Unity is handled by conditional compilcation.
Leaked HGlobal is allocated by member initialization statement, removed it.
| private static IntPtr m_pCallbackMsg; | ||
| private static int m_initCount; | ||
| // 4096 * 3 is enough for most api calls, | ||
| private static int s_currentCallResultBufferSize = 4096 * 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on this scales me a little bit
|
Any objections @JamesMcGhee @yaakov-h ? |
|
I'm a little bit concerned about thread safety and concurrency re. the static buffer, and also that there is no limit on the buffer until hitting INT_MAX and then crashing, particularly when this buffer stays around for the lifetime of the application. If use of ArrayPool/ObjectPool is available or similar then it might be better to reuse that system. Also throwing raw IntPtrs just kind of gives me the heebie-jeebies in general. |
Your concerns is right, but I see some usage of static shared buffers on CallbackMsg_t, so this might be acceptable for now. Buffer shrinking is also a good point, I plan to fix it by a counter about buffer too large. And also, crashing whole application will be fixed next. The exception thrown by reaching INT_MAX is handled and will only skip particular result.
Considering we still support some clients that don't have native |
|
Overall no objections to the concept not where I can run and test myself but read over the change set. As to the Thread Safty aspect On the overflow topic, it should have belts and braces to check for and handle overflow, there are a few ways you could implement recovering space it should be a non-issue there aren't that many CallResult calls that should be called so frequently that it would be a problem. That said there should never be an Access Violation or Overlow that is unhandled either not in C# anyway |
|
Wait, let me do some test first. |
|
Functional test under .NET 8 and .NET Standard 2.1 passed. |
|
Functional test code below, call me for making project setup public. // See https://aka.ms/new-console-template for more information
using Steamworks;
Console.WriteLine("Hello, World!");
CancellationTokenSource loopCts = new();
ulong modFAGE_id = 1579583001;
try
{
SteamAPI.Init();
Thread dispatcher = new(() =>
{
while (!loopCts.IsCancellationRequested)
SteamAPI.RunCallbacks();
});
dispatcher.IsBackground = true;
dispatcher.Start();
// Add test code here
CallResult<SteamUGCQueryCompleted_t>[] completionHolder = [.. GenerateReceivers(10, null!)];
for (int i = 0; i < completionHolder.Length; i++)
{
var queryHandle = SteamUGC.CreateQueryUGCDetailsRequest([new(modFAGE_id + (uint)i)], 1);
var queryApiCallHandle = SteamUGC.SendQueryUGCRequest(queryHandle);
completionHolder[i].Set(queryApiCallHandle);
}
SpinWait spinner = new();
while (Sync.CompletedJobsCount != 10)
{
spinner.SpinOnce();
}
}
finally
{
loopCts.Cancel();
SteamAPI.Shutdown();
}
static IEnumerable<CallResult<SteamUGCQueryCompleted_t>> GenerateReceivers(int amount, Barrier? completionBarrier)
{
// completionBarrier.AddParticipants(amount);
for (int i = 0; i < amount; i++)
{
yield return new((r, f) =>
{
if (f)
{
Console.WriteLine("API Invocation IO Failure");
Interlocked.Increment(ref Sync.CompletedJobsCount);
return;
}
var completedHandle = r.m_handle;
if (!SteamUGC.GetQueryUGCResult(completedHandle, 0, out var details))
{
Console.WriteLine("GetResult() generic failure");
Interlocked.Increment(ref Sync.CompletedJobsCount);
return;
}
if (!SteamUGC.GetQueryUGCStatistic(completedHandle, 0, EItemStatistic.k_EItemStatistic_NumComments, out ulong commentsCount))
{
Console.WriteLine("GetStatistic() generic failure");
Interlocked.Increment(ref Sync.CompletedJobsCount);
return;
}
Console.WriteLine($"Title: {details.m_rgchTitle}, URL: {details.m_rgchURL}, Comments count: {commentsCount}");
Interlocked.Increment(ref Sync.CompletedJobsCount);
return;
});
}
}
static class Sync
{
public static volatile int CompletedJobsCount = 0;
} |
82286de to
609733e
Compare
Introduce `TargetFrameworks` have critial effect to build process, which results breaking batch-build step of nuget pack. Considering side effect above introduced by adding .NET 8 support, to maintain packageing functions, abandon .NET 8 now.
In addition, original buggy size rounding is replaced with a more plain implementation.
|
I feel this version is more satisfied, ready to review. |
|
Ping for requesting review @yaakov-h @JamesMcGhee @rlabrecque |
Reduced HGlobal allocation times in CallResult scenario by cache IntPtr buffer .
Changed .NET Standard project to make use of new(.NET 5+) API, Unity will also get benefit from it. Compatibility to Framework and old Unity is handled by conditional compilcation.