-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
System.AppContext.GetData is not optimized for reads #94894
Comments
You'd need to follow the API request template in the separate issue for that. |
Yes, it sounds like a problem that should be fixed in SqlClient package.
AppContext/AppDomain.GetData/Data has always been a simple store. It is meant to be used for immutable data and the lookups are meant to be cached by the caller. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsDescriptionIn profiling our application under high load, we've found that calls to public static object? GetData(string name)
{
ArgumentNullException.ThrowIfNull(name);
if (s_dataStore == null)
return null;
object? data;
lock (s_dataStore)
{
s_dataStore.TryGetValue(name, out data);
}
return data;
} The way it's currently implemented is a static dictionary protected by a lock, as it's possible to modify the contents through Now ideally the SqlClient package does not use My suggestion would be:
Additionally, I would propose an API change to The same holds for I would be quite happy to try and make these changes myself, but first I would like to know if these are sensible changes. Perhaps my assumption that writes are rare is wrong.
|
Is the problem in https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs ? The code in this file has number of issues. It has static caches for all the values, but it does not actually use them the cache for two of them. Also, all lookups have race conditions. |
Yes, that's the one.
Good catches.
But in that case there's also no harm in making GetData faster, regardless of whether SqlClient should use it differently. I think the added complexity would be fairly minimal. |
You have been talking about making GetData faster by making SetData slower and using much more heavy weight data structures for it. It means that every app will have to pay for this optimization at startup, and trimmed or AOT compiled apps are going to pay for it by increase in binary size. It is not clear to me that it is a win. |
@jkotas My FrozenDictionary suggestion is more of a "nice to have", if we just stick to removing the lock and keep using Dictionary I don't think it will affect AOT except in maybe adding the constructor overload of Dictionary that takes another dictionary. As for whether or not it's a win, I think it really depends on how AppContext is used in practice. Is the SqlClient package an outlier, or are there many packages that use it like this? Are there also cases of SetData being called a lot? I don't know how to get this information though. But I think you're correct in saying that even from the SqlClient package it's unintentional in how it's currently used and I should definitely file a bug ticket for that project. From how frequently they called it I figured it was in case someone changed it at runtime, but indeed their intention also seems to be to cache it and never check it again. |
PR to cache the lookup is dotnet/SqlClient#2227 |
This was fixed by dotnet/SqlClient#2225 instead |
Description
In profiling our application under high load, we've found that calls to
System.AppContext.GetData(string)
make up ~3% of our total CPU time, making it a top 5 CPU consumer function overall for us. All calls to that method originate from the Microsoft.Data.SqlClient package, where it uses it for feature flag checking. It seems to do this a lot, deep inside the TDS parser, multiple times per query. See the implementation below.The way it's currently implemented is a static dictionary protected by a lock, as it's possible to modify the contents through
AppContext.SetData
. It seems likely to me that getting data fromAppContext
is used overwhelmingly more than setting data. I've set a breakpoint inSetData
and in our application it's never called, the dictionary is initialized at application startup and never touched, and I think this is going to be the case for a lot of applications. But the overhead of the lock is incurred for every read. And in an application doing thousands of concurrent queries it's enough to register as a hot path. At least I assume it's the highly contested lock that's the problem and not the dictionary lookup (the trace I have does not tell me).Now ideally the SqlClient package does not use
AppContext.GetData
this frequently but currently it does and I thinkAppContext.GetData
could probably be optimized for reads rather than writes.My suggestion would be:
SetData
is called, by copying the contents of the current dictionary to a new dictionary, protected by a lock.Additionally, I would propose an API change to
AppContext
that adds an event you can subscribe to to be notified of changes to data, to help packages like Microsoft.Data.SqlClient cache the values in simple boolean fields and only update them when necessary, instead of doing a still (relatively) expensive dictionary lookup.The same holds for
AppContext.TryGetSwitch
, which can also end up callingGetData
. It too also uses a locked dictionary, but in our case it's alwaysnull
and delegates toGetData
.I would be quite happy to try and make these changes myself, but first I would like to know if these are sensible changes. Perhaps my assumption that writes are rare is wrong.
The text was updated successfully, but these errors were encountered: