[Bug] Improve performance in Serialize and Deserialize methods in AdalCacheOperations

[pmaytak]

trafficstars

Which version of MSAL.NET are you using? Latest

Problem Serialize and Deserialize methods in AdalCacheOperations create a lot of LOH allocations.

Context AdalCacheOperations are used to serialize refresh tokens and enable reuse between ADAL and MSAL, which is not needed if only MSAL is used.

Workaround

• Call WithLegacyCacheCompatibility(false) to disable those ADAL related calls. It's a migration feature, which is not needed when using only MSAL.

Possible soliton Set the legacy cache compatibility to false by default in MSAL.

Possible solution Change Serialize to

        public static Stream Serialize(IDictionary<AdalTokenCacheKey, AdalResultWrapper> tokenCacheDictionary)
        {
            Stream stream = new SegmentedMemoryStream();

            BinaryWriter writer = new BinaryWriter(stream);
            writer.Write(SchemaVersion);

            writer.Write(tokenCacheDictionary.Count);

            StringBuilder reusedBuilder = new StringBuilder();

            foreach (KeyValuePair<AdalTokenCacheKey, AdalResultWrapper> kvp in tokenCacheDictionary)
            {
                writer.Write(string.Format(
                    CultureInfo.InvariantCulture,
                    "{1}{0}{2}{0}{3}{0}{4}",
                    Delimiter,
                    kvp.Key.Authority,
                    kvp.Key.Resource,
                    kvp.Key.ClientId,
                    (int)kvp.Key.TokenSubjectType));

                reusedBuilder.Clear();
                writer.Write(kvp.Value.Serialize(reusedBuilder));
            }

            stream.Position = 0;

            return stream;
        }

Change Deserialize to

            using (Stream stream = new MemoryStream(state))
            {
                BinaryReader reader = new BinaryReader(stream);
                int blobSchemaVersion = reader.ReadInt32();

                int count = reader.ReadInt32();
                for (int n = 0; n < count; n++)
                {
                    string keyString = reader.ReadString();

                    string[] kvpElements = keyString.Split(new[] { Delimiter }, StringSplitOptions.None);
                    AdalResultWrapper resultEx = AdalResultWrapper.Deserialize(reader.ReadString());
                    AdalTokenCacheKey key = new AdalTokenCacheKey(
                        kvpElements[0],
                        kvpElements[1],
                        kvpElements[2],
                        (TokenSubjectType)int.Parse(kvpElements[3], CultureInfo.CurrentCulture),
                        resultEx.Result.UserInfo);

                    dictionary.Add(key, resultEx);
                }
            }

[bgavrilMS]

I would rather deprecate ADAL cache compat completely. We can do it from CCA asap, probably from pca as well because pca can get SSO via WAM. @jmprieur thoughts?

[jmprieur]

I agree. When we take a major version, let's deprecate it

[henrik-me]

Any reason we are not marking it as deprecated now with a warning recommending to turn it off? Could save CPU and memory resources.

[bgavrilMS]

@henrik-me - this logic gets executed every time, unless we can figure out that the user has not enabled ADAL migration or that it can't be done, i.e.:

1. client_credentials flow (RT) or OBO (since tokens are tied to original assertion)
2. custom serialization is not used

We have discussed in the past to also disable it if a request does not use "SerializeADAL" APIs, but decided that it was too complex.

[bgavrilMS]

Looks like this is blocking some folks, so implement the suggestion.

[pmaytak]

Looks like this is blocking some folks, so implement the suggestion.

@bgavrilMS Which suggestion Dynamically disable this in code or Improve the Adal Serialize/Deserialize perf

Any reason we are not marking it as deprecated now with a warning recommending to turn it off? Could save CPU and memory resources.

There's no public API to obsolete, since this ADAL stuff is done internally by default? We could log a warning specifying to disable this or if they want it enabled, then explicitly call WithLegacyCacheCompatibility(true) which will remove the log warning too.

[bgavrilMS]

Improving the perf as suggested in the bug description seems like a quick win.

[bgavrilMS]

You'll need to find the original email to fix this. The SegmentedMemoryStream is not part of .NET

[neha-bhargava]

Tried to apply the performance optimization suggestions and they are causing cache compat test failures. Closing as won't fix.