Reduce allocations in MediaTokenService and improve performance #10941

lahma · 2022-01-02T10:35:31Z

One has to have hobbies, right? Felt like a fun exercise as Orchard can use all .NET 6 goodness.

MediaTokenService does some unnecessary allocations and is called on every request which has images served.

~~tweak constructor to iterate array, DI gives array and it's fastest to traverse - constructor takes considerable amount of time (see scope note below)~~
- no need after changing service to singleton, shaved off extra 200ns without the constructor invoke
don't ToString StringValues, keep them as-is as in dictionary once built
parse commands and others commands with one pass using constructs that QueryHelpers uses
allocate static command arrays to return from processors, some of them were always returning new ones
change GetHash not to allocation anonymous closure each time by using Try/Set construct, uses same logic that extension method GetOrCreate uses
change hash calculation to use stackalloc and spans for workloads < 1024 bytes (common case)
use custom AddQueryString method that understands spans and concrete dictionary type

MediaTokenService seems to be scoped service and thus known commands need to be initialized for every request, is this a tenancy things and cannot be made singleton?

Benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=6.0.101
  [Host]     : .NET 6.0.1 (6.0.121.56705), X64 RyuJIT
  DefaultJob : .NET 6.0.1 (6.0.121.56705), X64 RyuJIT

Before

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
AddTokenToPath	1.335 us	0.0097 us	0.0091 us	0.1984	-	3 KB
AddTokenToPath_NoCache	1.993 us	0.0089 us	0.0083 us	0.2441	-	4 KB
AddTokenToPath_LongPath	2.474 us	0.0157 us	0.0139 us	0.4005	0.0038	7 KB
AddTokenToPath_LongPath_NoCache	3.135 us	0.0098 us	0.0091 us	0.4578	0.0038	7 KB

After

Now testing with singleton - unlike main branch!

Method	Mean	Error	StdDev	Gen 0	Allocated
AddTokenToPath	696.1 ns	1.18 ns	1.05 ns	0.0715	1 KB
AddTokenToPath_NoCache	1,309.6 ns	2.28 ns	1.91 ns	0.1087	2 KB
AddTokenToPath_LongPath	1,556.8 ns	4.93 ns	4.37 ns	0.2060	3 KB
AddTokenToPath_LongPath_NoCache	2,248.9 ns	6.70 ns	6.27 ns	0.2441	4 KB

lahma · 2022-01-02T10:35:52Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

@@ -15,7 +18,7 @@ public class MediaTokenService : IMediaTokenService
        private const string TokenCacheKeyPrefix = "MediaToken:";
        private readonly IMemoryCache _memoryCache;

-        private readonly HashSet<string> _knownCommands = new HashSet<string>();
+        private readonly HashSet<string> _knownCommands = new(12);


12 is what default Orchard setup seems to have

lahma · 2022-01-02T10:36:54Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

            }

            // Using the command values as a key retrieve from cache
-            var queryStringTokenKey = CreateQueryStringTokenKey(processingCommands.Values);
+            var queryStringTokenKey = CreateQueryStringTokenKey(processingCommands);


.Values allocates new ValueCollection, no need for that so let's just pass the dictionary which has efficient enumeration

lahma · 2022-01-02T10:38:15Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

+            return AddQueryString(path.AsSpan(0, pathIndex), processingCommands);
+        }
+
+        private void ParseQuery(


same as in QueryHelpers, but produces two dictionaries based on whether known commands matches

lahma · 2022-01-02T10:38:59Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

+        /// <summary>
+        /// Specialized version with fast enumeration and no StringValues string allocation.
+        /// </summary>
+        private static string CreateQueryStringTokenKey(Dictionary<string, StringValues> values)


a bit of duplication, but with concrete type overload we get fast struct enumerator instead of one wrapped via interface

lahma · 2022-01-02T10:39:43Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

-               {
-                   entry.SlidingExpiration = TimeSpan.FromHours(5);
+        {
+            if (!_memoryCache.TryGetValue(queryStringTokenKey, out var result))


old code had capturing closure as extension method took a Func which needed outside context, now using methods actually present in IMemoryCache to prevent that

lahma · 2022-01-02T10:40:34Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

+        /// Custom version of <see cref="QueryHelpers.AddQueryString(string,string,string)"/> that takes our pre-built
+        /// dictionary, uri as ReadOnlySpan&lt;char&gt; and uses ZString. Otherwise same logic.
+        /// </summary>
+        private static string AddQueryString(


this ones copes with ReadOnlySpan<char> and uses ZString instead of allocating StringBuilder

deanmarcussen · 2022-01-02T11:46:45Z

One has to have hobbies, right? Felt like a fun exercise as Orchard can use all .NET 6 goodness.

Indeed :)

We aren't quite there with .NET 6 only yet, still multi targetting. Due to drop 3.1 and 5 after the next release (due shortly)

MediaTokenService seems to be scoped service and thus known commands need to be initialized for every request, is this a tenancy things and cannot be made singleton?

No, it's just Scoped until you find value in making it a Singleton.

lahma · 2022-01-02T13:03:18Z

One has to have hobbies, right? Felt like a fun exercise as Orchard can use all .NET 6 goodness.

Indeed :)

We aren't quite there with .NET 6 only yet, still multi targetting. Due to drop 3.1 and 5 after the next release (due shortly)

I did a workaround with #if/#else that's the slower path without the zero-allocating query params traversal.

MediaTokenService seems to be scoped service and thus known commands need to be initialized for every request, is this a tenancy things and cannot be made singleton?

No, it's just Scoped until you find value in making it a Singleton.

Thanks for the confirmation. I changed it to singleton and tested in PR with singleton instance, which gave 930ns -> 700ns change. Quite big difference I'd say just by singletoning the instance.

lahma · 2022-01-02T13:06:48Z

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs

+            out Dictionary<string, StringValues> processingCommands,
+            out Dictionary<string, StringValues> otherCommands)
+        {
+            var parsed = QueryHelpers.ParseQuery(queryString);


basically old logic which generates at least two dictionaries

* keep query StringValues as-is instead of ToString as string * use single pass to handle known commands and other commands * allocate static command arrays for processors * use spans and stack alloc for hash calculation * make MediaTokenService singleton

lahma · 2022-01-02T13:37:01Z

I tweaked existing test case to test both with known and unknown commands and it now checks for complete generated string, results didn't change between main and this branch.

lahma commented Jan 2, 2022

View reviewed changes

lahma force-pushed the reduce-media-token-service-allocations branch from 8b8676d to 8a0e8b2 Compare January 2, 2022 12:54

lahma commented Jan 2, 2022

View reviewed changes

lahma added 2 commits January 2, 2022 15:30

add more test coverage to ensure we didn't break anything

9756671

lahma force-pushed the reduce-media-token-service-allocations branch from 8a0e8b2 to 9756671 Compare January 2, 2022 13:31

sebastienros merged commit 974e00e into OrchardCMS:main Jan 8, 2022

lahma deleted the reduce-media-token-service-allocations branch January 8, 2022 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations in MediaTokenService and improve performance #10941

Reduce allocations in MediaTokenService and improve performance #10941

lahma commented Jan 2, 2022 •

edited

Loading

lahma Jan 2, 2022

lahma Jan 2, 2022

lahma Jan 2, 2022

lahma Jan 2, 2022

lahma Jan 2, 2022

lahma Jan 2, 2022

deanmarcussen commented Jan 2, 2022

lahma commented Jan 2, 2022

lahma Jan 2, 2022

lahma commented Jan 2, 2022

Reduce allocations in MediaTokenService and improve performance #10941

Reduce allocations in MediaTokenService and improve performance #10941

Conversation

lahma commented Jan 2, 2022 • edited Loading

Benchmarks

Before

After

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma Jan 2, 2022

Choose a reason for hiding this comment

deanmarcussen commented Jan 2, 2022

lahma commented Jan 2, 2022

lahma Jan 2, 2022

Choose a reason for hiding this comment

lahma commented Jan 2, 2022

lahma commented Jan 2, 2022 •

edited

Loading