System.AppContext.GetData is not optimized for reads

[JulianRooze]

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.

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 AppContext.SetData. It seems likely to me that getting data from AppContext is used overwhelmingly more than setting data. I've set a breakpoint in SetData 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 think AppContext.GetData could probably be optimized for reads rather than writes.

My suggestion would be:

1. Use copy-on-write on the off-chance that SetData is called, by copying the contents of the current dictionary to a new dictionary, protected by a lock.
2. This seems like a good fit to swap the regular dictionary for the new FrozenDictionary to squeeze out a little more read performance.

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 calling GetData. It too also uses a locked dictionary, but in our case it's always null and delegates to GetData.

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.

[MichalPetryka]

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.

You'd need to follow the API request template in the separate issue for that.

[jkotas]

Now ideally the SqlClient package does not use AppContext.GetData this frequently but currently it does

Yes, it sounds like a problem that should be fixed in SqlClient package.

I think AppContext.GetData could probably be optimized for reads rather than writes.

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.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

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.

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 AppContext.SetData. It seems likely to me that getting data from AppContext is used overwhelmingly more than setting data. I've set a breakpoint in SetData 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 think AppContext.GetData could probably be optimized for reads rather than writes.

My suggestion would be:

1. Use copy-on-write on the off-chance that SetData is called, by copying the contents of the current dictionary to a new dictionary, protected by a lock.
2. This seems like a good fit to swap the regular dictionary for the new FrozenDictionary to squeeze out a little more read performance.

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 calling GetData. It too also uses a locked dictionary, but in our case it's always null and delegates to GetData.

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.

Author:	JulianRooze
Assignees:	-
Labels:	area-System.Runtime, tenet-performance, untriaged, needs-area-label
Milestone:	-

[jkotas]

Microsoft.Data.SqlClient package, where it uses it for feature flag checking.

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.

[JulianRooze]

@jkotas

Is the problem in https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs ?

Yes, that's the one.

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.

Good catches.

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.

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.

[jkotas]

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.

[JulianRooze]

@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.

[Wraith2]

Yes, it sounds like a problem that should be fixed in SqlClient package.

PR to cache the lookup is dotnet/SqlClient#2227

[JulianRooze]

@jkotas thanks for your input and thank you @Wraith2 for implementing it :)