Use OPTIMIZE FOR UNKNOWN for dependents queries (#8112)

* Use OPTIMIZE FOR UNKNOWN for dependents queries
* Add query hint configuration to RECOMPILE for some package IDs

Progress on #8078

Author: joelverhagen

src/NuGetGallery.Services/Configuration/IQueryHintConfiguration.cs

@@ -0,0 +1,16 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+namespace NuGetGallery.Services
+{
+    public interface IQueryHintConfiguration
+    {
+        /// <summary>
+        /// Determines whether the RECOMPILE query hint should be used for the package dependents query. Some popular
+        /// package IDs perform much better with their own dedicated query plan instead of OPTIMIZE FOR UNKNOWN.
+        /// </summary>
+        /// <param name="packageId">The package ID.</param>
+        /// <returns>True, if the RECOMPILE query hint should be used for the provided package ID.</returns>
+        bool ShouldUseRecompileForPackageDependents(string packageId);
+    }
+}

src/NuGetGallery.Services/Configuration/QueryHintConfiguration.cs

@@ -0,0 +1,32 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using Newtonsoft.Json;
+
+namespace NuGetGallery.Services
+{
+    public class QueryHintConfiguration : IQueryHintConfiguration
+    {
+        public QueryHintConfiguration() : this(Enumerable.Empty<string>())
+        {
+        }
+
+        [JsonConstructor]
+        public QueryHintConfiguration(IEnumerable<string> recompileForPackageDependents)
+        {
+            RecompileForPackageDependents = new HashSet<string>(
+                recompileForPackageDependents ?? Enumerable.Empty<string>(),
+                StringComparer.OrdinalIgnoreCase);
+        }
+
+        public HashSet<string> RecompileForPackageDependents { get; }
+
+        public bool ShouldUseRecompileForPackageDependents(string packageId)
+        {
+            return RecompileForPackageDependents.Contains(packageId);
+        }
+    }
+}

src/NuGetGallery.Services/NuGetGallery.Services.csproj

@@ -116,12 +116,14 @@
     <Compile Include="Configuration\ILoginDiscontinuationConfiguration.cs" />
     <Compile Include="Configuration\IPackageDeleteConfiguration.cs" />
     <Compile Include="Configuration\IODataCacheConfiguration.cs" />
+    <Compile Include="Configuration\IQueryHintConfiguration.cs" />
     <Compile Include="Configuration\IServiceBusConfiguration.cs" />
     <Compile Include="Configuration\ISymbolsConfiguration.cs" />
     <Compile Include="Configuration\ITyposquattingConfiguration.cs" />
     <Compile Include="Configuration\LoginDiscontinuationConfiguration.cs" />
     <Compile Include="Configuration\ODataCacheConfiguration.cs" />
     <Compile Include="Configuration\PackageDeleteConfiguration.cs" />
+    <Compile Include="Configuration\QueryHintConfiguration.cs" />
     <Compile Include="Configuration\SecretReader\EmptySecretReaderFactory.cs" />
     <Compile Include="Configuration\SecretReader\SecretReaderFactory.cs" />
     <Compile Include="Configuration\ServiceBusConfiguration.cs" />

src/NuGetGallery.Services/PackageManagement/PackageService.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Data.Common;
 using System.Data.Entity;
 using System.IO;
 using System.Linq;
@@ -26,6 +25,7 @@ public class PackageService : CorePackageService, IPackageService
         private readonly ITelemetryService _telemetryService;
         private readonly ISecurityPolicyService _securityPolicyService;
         private readonly IEntitiesContext _entitiesContext;
+        private readonly IContentObjectService _contentObjectService;
         private const int packagesDisplayed = 5;
 
         public PackageService(
@@ -35,13 +35,15 @@ public PackageService(
             IAuditingService auditingService,
             ITelemetryService telemetryService,
             ISecurityPolicyService securityPolicyService,
-            IEntitiesContext entitiesContext)
+            IEntitiesContext entitiesContext,
+            IContentObjectService contentObjectService)
             : base(packageRepository, packageRegistrationRepository, certificateRepository)
         {
             _auditingService = auditingService ?? throw new ArgumentNullException(nameof(auditingService));
             _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             _securityPolicyService = securityPolicyService ?? throw new ArgumentNullException(nameof(securityPolicyService));
             _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext));
+            _contentObjectService = contentObjectService ?? throw new ArgumentNullException(nameof(contentObjectService));
         }
 
         /// <summary>
@@ -155,8 +157,34 @@ public PackageDependents GetPackageDependents(string id)
             }
 
             PackageDependents result = new PackageDependents();
-            
-            using (_entitiesContext.WithQueryHint("RECOMPILE"))
+
+            // We use OPTIMIZE FOR UNKNOWN by default here because there are distinct 2-3 query plans that may be
+            // selected via SQL Server parameter sniffing. Because SQL Server caches query plans, this means that the
+            // first parameter plus query combination that SQL sees defines which query plan is selected and cached for
+            // all subsequent parameter values of the same query. This could result in a non-optimal query plan getting
+            // cached depending on what package ID is viewed first. Using OPTIMIZE FOR UNKNOWN causes a predictable
+            // query plan to be cached.
+            // 
+            // For example, the query plan for Newtonsoft.Json is very good for that specific parameter value since
+            // there are so many package dependents but the same query plan takes a very long time for packages with few
+            // or no dependents. The query plan for "UNKNOWN" (that is a package ID with unknown SQL Server statistic)
+            // behaves somewhat poorly for Newtonsoft.Json (2-5 seconds) but very well for the vast majority of
+            // packages. Because we have in-memory caching above this layer, OPTIMIZE FOR UNKNOWN is acceptable other
+            // unconfigured cases similar to Newtonsoft.Json because the extra cost of the non-optimal query plan is
+            // amortized over many, many page views. For the long tail packages, in-memory caching is less effective
+            // (low page views) so an optimal query should be selected for this category.
+            //
+            // For the cases where RECOMPILE is known to perform the best, the package ID can be added to the query hint
+            // configuration JSON file from the content object service. This should only be done when the following
+            // things are true:
+            //
+            //   1. The overhead of SQL Server recompile is worth it. We have seen the overhead to be 5-50ms.
+            //   2. SQL Server has up to date statistics which will lead to the proper query plan being selected.
+            //   3. SQL Server actually picks the proper query plan. We have observed cases where this does not happen
+            //      even with up-to-date statistics.
+            //
+            var useRecompile = _contentObjectService.QueryHintConfiguration.ShouldUseRecompileForPackageDependents(id);
+            using (_entitiesContext.WithQueryHint(useRecompile ? "RECOMPILE" : "OPTIMIZE FOR UNKNOWN"))
             {
                 result.TopPackages = GetListOfDependents(id);
                 result.TotalPackageCount = GetDependentCount(id);

src/NuGetGallery.Services/ServicesConstants.cs

@@ -56,6 +56,7 @@ public static class ContentNames
             public static readonly string ABTestConfiguration = "AB-Test-Configuration";
             public static readonly string ODataCacheConfiguration = "OData-Cache-Configuration";
             public static readonly string CacheConfiguration = "Cache-Configuration";
+            public static readonly string QueryHintConfiguration = "Query-Hint-Configuration";
         }
     }
 }

src/NuGetGallery.Services/Storage/ContentObjectService.cs

@@ -27,6 +27,7 @@ public ContentObjectService(IContentService contentService)
             ABTestConfiguration = new ABTestConfiguration();
             ODataCacheConfiguration = new ODataCacheConfiguration();
             CacheConfiguration = new CacheConfiguration();
+            QueryHintConfiguration = new QueryHintConfiguration();
         }
 
         public ILoginDiscontinuationConfiguration LoginDiscontinuationConfiguration { get; private set; }
@@ -37,6 +38,7 @@ public ContentObjectService(IContentService contentService)
         public IABTestConfiguration ABTestConfiguration { get; private set; }
         public IODataCacheConfiguration ODataCacheConfiguration { get; private set; }
         public ICacheConfiguration CacheConfiguration { get; private set; }
+        public IQueryHintConfiguration QueryHintConfiguration { get; private set; }
 
         public async Task Refresh()
         {
@@ -72,6 +74,10 @@ await Refresh<ODataCacheConfiguration>(ServicesConstants.ContentNames.ODataCache
             CacheConfiguration =
                 await Refresh<CacheConfiguration>(ServicesConstants.ContentNames.CacheConfiguration) ??
                 new CacheConfiguration();
+
+            QueryHintConfiguration =
+                await Refresh<QueryHintConfiguration>(ServicesConstants.ContentNames.QueryHintConfiguration) ??
+                new QueryHintConfiguration();
         }
 
         private async Task<T> Refresh<T>(string contentName)

src/NuGetGallery.Services/Storage/IContentObjectService.cs

@@ -16,6 +16,7 @@ public interface IContentObjectService
         IABTestConfiguration ABTestConfiguration { get; }
         IODataCacheConfiguration ODataCacheConfiguration { get; }
         ICacheConfiguration CacheConfiguration { get; }
+        IQueryHintConfiguration QueryHintConfiguration { get; }
 
         Task Refresh();
     }

src/NuGetGallery/App_Data/Files/Content/Query-Hint-Configuration.json

@@ -0,0 +1,3 @@
+{
+  "RecompileForPackageDependents": []
+}

src/NuGetGallery/NuGetGallery.csproj

@@ -2302,6 +2302,9 @@
   <ItemGroup>
     <Content Include="App_Data\Files\Content\Cache-Configuration.json" />
   </ItemGroup>
+  <ItemGroup>
+    <Content Include="App_Data\Files\Content\Query-Hint-Configuration.json" />
+  </ItemGroup>
   <PropertyGroup>
     <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">10.0</VisualStudioVersion>
     <VSToolsPath Condition="'$(VSToolsPath)' == ''">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)</VSToolsPath>

src/NuGetGallery/Web.config

@@ -360,7 +360,7 @@
     </customErrors>
     <sessionState mode="Off"/>
     <machineKey configProtectionProvider="GalleryMachineKeyConfigurationProvider">
-      <EncryptedData />
+      <EncryptedData/>
     </machineKey>
   </system.web>
   <system.webServer>