String performance implementation improvements #2217

mitchelsellers · 2018-08-08T20:22:54Z

Improvements to the management and processing of strings. This change was limited to the changes that can be made that would not change behavior, or impact the possibility of a behavior change.

As such, the changes here are not "perfect," but represent the best efforts to improve the operation with strings in the core. The goals here are to avoid calls that result in new strings being generated via calls such as ToLower(), ToUpper() etc and comparisons being made.

The following constructs were followed when making these changes.

Implementations that used ToUpperInvariant() or ToLowerInvariant() were moved to use StringComparison.InvariantCultureIgnoreCase
Comparisons that were done before with format specifiers but redundant calls to ToLower()/ToUpper() removed

I did not fix the following types of items that at some point in the future might be worth continuing to research.

ToLowerInvariant().Contains("") = These should be changed to IndexOf() if possible to avoid the string creation of ToLowerInvariant()
Situations where the same variable was called multiple times convertng the value
Situations where performance could be further improved by using OrdinalIgnoreCase rather than InvariantCultureIgnoreCase
Unique situation where both invariant & Ordnial was used. (DotNetNuke.Services.FileSystem.FolderManager)

In testing the updated installation with these changes I am seeing an exponential reduction in the number of string objects in memory snapshots compared to the baseline version, with indications of less garbage collector cycles. No adverse implementation details.

Fixes #2215

david-poindexter · 2018-08-08T21:19:45Z

I believe this should reference #2215 instead. Excellent work @mitchelsellers!

bdukes

Thanks, @mitchelsellers, looks solid to me

bdukes · 2018-08-08T20:27:39Z

DNN Platform/DotNetNuke.Web/Common/DotNetNukeShutdownOverload.cs

@@ -153,7 +153,7 @@ private static void WatcherOnChanged(object sender, FileSystemEventArgs e)
            if (Logger.IsInfoEnabled && !e.FullPath.EndsWith(".log.resources"))
                Logger.Info($"Watcher Activity: {e.ChangeType}. Path: {e.FullPath}");

-            if (_handleShutdowns && !_shutdownInprogress && (e.FullPath ?? "").ToLowerInvariant().StartsWith(_binFolder))
+            if (_handleShutdowns && !_shutdownInprogress && (e.FullPath ?? "").StartsWith(_binFolder, StringComparison.InvariantCultureIgnoreCase))


Does this change mean we can remove the ToLowerInvariant call on _binFolder?

I left that call as-is as I wasn't aware of the inherit risk of changing it on the file system watcher.

bdukes · 2018-08-08T20:29:13Z

DNN Platform/DotNetNuke.WebUtility/ClientAPI.vb

@@ -557,7 +557,7 @@ Namespace DotNetNuke.UI.Utilities
        End Function

        Public Shared Function IsInCallback(ByVal objPage As Page) As Boolean
-            Return Len(objPage.Request(SCRIPT_CALLBACKID)) > 0 AndAlso objPage.Request.HttpMethod.ToUpper() = "POST"
+            Return Len(objPage.Request(SCRIPT_CALLBACKID)) > 0 AndAlso objPage.Request.HttpMethod.Equals("POST", StringComparison.OrdinalIgnoreCase)


the previous behavior of this call would have been culture specific, rather than Ordinal, right?

Correct so ordinal sorting seemed most appropriate, I could adjust if desired

bdukes · 2018-08-08T20:31:07Z

DNN Platform/Library/Common/Globals.cs

@@ -3889,7 +3889,7 @@ public static string GetDBConnectionString()
                    strExtension = File.Substring(File.LastIndexOf(".") + 1);
                }
                string FileName = File.Substring(CurrentDirectory.FullName.Length);
-                if (strExtensions.ToUpper().IndexOf(strExtension.ToUpper()) != -1 || string.IsNullOrEmpty(strExtensions))
+                if (strExtensions.IndexOf(strExtension, StringComparison.InvariantCultureIgnoreCase) != -1 || string.IsNullOrEmpty(strExtensions))


this original behavior would have been culture-specific, is there any real scenario where that matters?

Not from what I could tell, and being invariant seems to lesson any risk

bdukes · 2018-08-08T20:32:33Z

DNN Platform/Library/Data/SqlDataProvider.cs

@@ -199,16 +199,18 @@ private string GetConnectionStringUserID()
            string DBUser = "public";

            //If connection string does not use integrated security, then get user id.
-            if (ConnectionString.ToUpper().Contains("USER ID") || ConnectionString.ToUpper().Contains("UID") || ConnectionString.ToUpper().Contains("USER"))
+            //Normalize to uppervase before all of the comparisons


typo: uppervase for uppercase

bdukes · 2018-08-08T21:14:13Z

DNN Platform/Library/Entities/Urls/AdvancedUrlRewriter.cs

@@ -2338,8 +2338,9 @@ private static bool IgnoreRequestForInstall(string physicalPath, string refererP

        private static bool IgnoreRequestForWebServer(string requestedPath)
        {
-            if (requestedPath.ToLowerInvariant().IndexOf("synchronizecache.aspx", StringComparison.Ordinal) > 1
-                || requestedPath.EndsWith("keepalive.aspx", true, CultureInfo.InvariantCulture))
+            //Should standardize comparison methods


I'm not sure I understand this comment

One uses EndsWith, the other looks for it anywhere. I think both could be safe in the future with ends with

I was assuming that synchronizecache might have parameters, whereas keepalive is not expected to

Correct, synchronizecache always has params. Keepalive shouldn't, but it might since some services add a random number param on requests to avoid caching. I think changing it to indexof would be best.

bdukes · 2018-08-08T21:14:36Z

DNN Platform/Library/Entities/Urls/AdvancedUrlRewriter.cs

-                || requestedPath.EndsWith("keepalive.aspx", true, CultureInfo.InvariantCulture))
+            //Should standardize comparison methods
+            if (requestedPath.IndexOf("synchronizecache.aspx", StringComparison.OrdinalIgnoreCase) > 1
+                || requestedPath.EndsWith("keepalive.aspx", StringComparison.OrdinalIgnoreCase))


should this one have switched from InvariantCulture to Ordinal?

THe thought here was standardization of the comparisons between the two calls

ohine

Amazing work Mitch! I can't wait to see the final numbers on performance improvements with these changes. I'll leave it up to you if you agree to change the one function I called out. I totally understand if we want to do that in a future release to reduce any risks of a change in behavior.

ohine · 2018-08-09T01:24:51Z

DNN Platform/Library/Common/Utilities/PathUtils.cs

+                   folderPath.Equals("skins", StringComparison.InvariantCultureIgnoreCase) ||
+                   folderPath.Equals("containers", StringComparison.InvariantCultureIgnoreCase) ||
+                   folderPath.StartsWith("skins/", StringComparison.InvariantCultureIgnoreCase) ||
+                   folderPath.StartsWith("containers/", StringComparison.InvariantCultureIgnoreCase);


This just seems odd. Wouldn't it make more sense to switch to just two checks for contains instead of doing an equals or startswith?

ohine · 2018-08-09T01:28:16Z

DNN Platform/Library/Data/SqlDataProvider.cs

@@ -199,16 +199,18 @@ private string GetConnectionStringUserID()
            string DBUser = "public";

            //If connection string does not use integrated security, then get user id.
-            if (ConnectionString.ToUpper().Contains("USER ID") || ConnectionString.ToUpper().Contains("UID") || ConnectionString.ToUpper().Contains("USER"))
+            //Normalize to uppervase before all of the comparisons


ohine · 2018-08-09T01:31:16Z

DNN Platform/Library/Entities/Urls/AdvancedUrlRewriter.cs

@@ -2338,8 +2338,9 @@ private static bool IgnoreRequestForInstall(string physicalPath, string refererP

        private static bool IgnoreRequestForWebServer(string requestedPath)
        {
-            if (requestedPath.ToLowerInvariant().IndexOf("synchronizecache.aspx", StringComparison.Ordinal) > 1
-                || requestedPath.EndsWith("keepalive.aspx", true, CultureInfo.InvariantCulture))
+            //Should standardize comparison methods


Correct, synchronizecache always has params. Keepalive shouldn't, but it might since some services add a random number param on requests to avoid caching. I think changing it to indexof would be best.

String performance implementation improvements

21f7865

mitchelsellers added the Community Legacy label used to identify community contributions label Aug 8, 2018

mitchelsellers requested review from bdukes and ohine August 8, 2018 20:22

bdukes approved these changes Aug 8, 2018

View reviewed changes

fix typo

5b951f0

ohine approved these changes Aug 9, 2018

View reviewed changes

ohine added this to the 9.2.2 milestone Aug 9, 2018

ohine added the Noteworthy label Aug 9, 2018

ohine merged commit 2049d2c into dnnsoftware:development Aug 14, 2018

Uh oh!

String performance implementation improvements #2217

String performance implementation improvements #2217

Uh oh!

Conversation

mitchelsellers commented Aug 8, 2018 • edited by bdukes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-poindexter commented Aug 8, 2018

Uh oh!

bdukes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mitchelsellers commented Aug 8, 2018 •

edited by bdukes

Loading