Skip to content

Various string manipulation optimizations #1543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 20, 2018
Merged

Various string manipulation optimizations #1543

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f6942b
Avoid code duplication for Root determination - added IsNotRoot
bahusoid Jan 19, 2018
e23bf84
string.IndexOf ordinal comparison where applicapable
bahusoid Jan 19, 2018
14fde54
Avoid full string conversion
bahusoid Jan 19, 2018
a6c9fec
SqlString avoid ToString conversion
bahusoid Jan 19, 2018
6bfe359
Generalized HqlVariable determination - introduced ParserHelper.HasHq…
bahusoid Jan 19, 2018
83e7c17
Added suggested optimizations and some more
bahusoid Jan 20, 2018
85583b9
Added CallableParser.HasReturnParameter with suggested optimizations
bahusoid Jan 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/NHibernate/AdoNet/Util/BasicFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ private static bool IsFunctionName(string tok)

private static bool IsWhitespace(string token)
{
return StringHelper.WhiteSpace.IndexOf(token) >= 0;
return StringHelper.WhiteSpace.IndexOf(token, StringComparison.Ordinal) >= 0;
}

private void Newline()
Expand All @@ -451,4 +451,4 @@ public void Dispose()

#endregion
}
}
}
9 changes: 5 additions & 4 deletions src/NHibernate/AdoNet/Util/DdlFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Text;
using NHibernate.Util;
Expand All @@ -18,15 +19,15 @@ public class DdlFormatter: IFormatter
/// </summary>
public virtual string Format(string sql)
{
if (sql.ToLowerInvariant().StartsWith("create table"))
if (sql.StartsWith("create table", StringComparison.OrdinalIgnoreCase))
{
return FormatCreateTable(sql);
}
else if (sql.ToLowerInvariant().StartsWith("alter table"))
else if (sql.StartsWith("alter table", StringComparison.OrdinalIgnoreCase))
{
return FormatAlterTable(sql);
}
else if (sql.ToLowerInvariant().StartsWith("comment on"))
else if (sql.StartsWith("comment on", StringComparison.OrdinalIgnoreCase))
{
return FormatCommentOn(sql);
}
Expand Down Expand Up @@ -138,4 +139,4 @@ private static bool IsQuote(string token)
return "\"".Equals(token) || "`".Equals(token) || "]".Equals(token) || "[".Equals(token) || "'".Equals(token);
}
}
}
}
2 changes: 1 addition & 1 deletion src/NHibernate/Cfg/XmlHbmBinding/CollectionBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ private void BindCollection(ICollectionPropertiesMapping collectionMapping, Mapp

//ORPHAN DELETE (used for programmer error detection)
var cascadeAtt = collectionMapping.Cascade;
if (!string.IsNullOrEmpty(cascadeAtt) && cascadeAtt.IndexOf("delete-orphan") >= 0)
if (!string.IsNullOrEmpty(cascadeAtt) && cascadeAtt.IndexOf("delete-orphan", StringComparison.Ordinal) >= 0)
model.HasOrphanDelete = true;

// GENERIC
Expand Down
20 changes: 11 additions & 9 deletions src/NHibernate/Engine/Query/CallableParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ public static Detail Parse(string sqlString)
{
Detail callableDetail = new Detail();

callableDetail.IsCallable = sqlString.IndexOf("{") == 0 &&
sqlString.IndexOf("}") == (sqlString.Length - 1) &&
sqlString.IndexOf("call") > 0;
int indexOfCall = -1;
callableDetail.IsCallable = sqlString.Length > 5 && // to be able to check sqlString[0] we at least need to make sure that string has at least 1 character. The simplest case all other conditions are true is "{call}" which is 6 characters, so check it.
sqlString[0] == '{' &&
sqlString[sqlString.Length - 1] == '}' &&
(indexOfCall = sqlString.IndexOf("call", StringComparison.Ordinal)) > 0;

if (!callableDetail.IsCallable)
return callableDetail;
Expand All @@ -35,13 +37,13 @@ public static Detail Parse(string sqlString)

callableDetail.FunctionName = functionMatch.Groups[1].Value;

callableDetail.HasReturn = sqlString.IndexOf("call") > 0 &&
sqlString.IndexOf("?") > 0 &&
sqlString.IndexOf("=") > 0 &&
sqlString.IndexOf("?") < sqlString.IndexOf("call") &&
sqlString.IndexOf("=") < sqlString.IndexOf("call");
callableDetail.HasReturn = indexOfCall > 0 &&
sqlString.IndexOf('?') > 0 &&
sqlString.IndexOf('=') > 0 &&
sqlString.IndexOf('?') < indexOfCall &&
sqlString.IndexOf('=') < indexOfCall;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even go as far as:

var indexOfQuestionMark = -1;
var indexOfEqual = -1;
callableDetail.HasReturn =
	indexOfCall > 0 &&
		(indexOfQuestionMark = sqlString.IndexOf('?')) > 0 &&
		(indexOfEqual = sqlString.IndexOf('=')) > 0 &&
		indexOfQuestionMark < indexOfCall &&
		indexOfEqual < indexOfCall;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Will do


return callableDetail;
}
}
}
}
4 changes: 2 additions & 2 deletions src/NHibernate/Engine/Query/NativeSQLQueryPlan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public int PerformExecuteUpdate(QueryParameters queryParameters, ISessionImpleme
private SqlString ExpandDynamicFilterParameters(SqlString sqlString, ICollection<IParameterSpecification> parameterSpecs, ISessionImplementor session)
{
var enabledFilters = session.EnabledFilters;
if (enabledFilters.Count == 0 || sqlString.ToString().IndexOf(ParserHelper.HqlVariablePrefix) < 0)
if (enabledFilters.Count == 0 || !ParserHelper.HasHqlVariable(sqlString))
{
return sqlString;
}
Expand All @@ -143,7 +143,7 @@ private SqlString ExpandDynamicFilterParameters(SqlString sqlString, ICollection

foreach (string token in tokens)
{
if (token.StartsWith(ParserHelper.HqlVariablePrefix))
if (ParserHelper.IsHqlVariable(token))
{
string filterParameterName = token.Substring(1);
string[] parts = StringHelper.ParseFilterParameterName(filterParameterName);
Expand Down
19 changes: 10 additions & 9 deletions src/NHibernate/Engine/Query/ParameterParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,12 @@ private ParameterParser()
public static void Parse(string sqlString, IRecognizer recognizer)
{
// TODO: WTF? "CALL"... it may work for ORACLE but what about others RDBMS ? (by FM)
bool hasMainOutputParameter = sqlString.IndexOf("call") > 0 &&
sqlString.IndexOf("?") > 0 &&
sqlString.IndexOf("=") > 0 &&
sqlString.IndexOf("?") < sqlString.IndexOf("call") &&
sqlString.IndexOf("=") < sqlString.IndexOf("call");
var indexOfCall = sqlString.IndexOf("call", StringComparison.Ordinal);
bool hasMainOutputParameter = indexOfCall > 0 &&
sqlString.IndexOf('?') > 0 &&
sqlString.IndexOf('=') > 0 &&
sqlString.IndexOf('?') < indexOfCall &&
sqlString.IndexOf('=') < indexOfCall;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous.

bool foundMainOutputParam = false;

int stringLength = sqlString.Length;
Expand All @@ -59,7 +60,7 @@ public static void Parse(string sqlString, IRecognizer recognizer)
// check comments
if (indx + 1 < stringLength && sqlString.Substring(indx,2) == "/*")
{
var closeCommentIdx = sqlString.IndexOf("*/", indx+2);
var closeCommentIdx = sqlString.IndexOf("*/", indx + 2, StringComparison.Ordinal);
recognizer.Other(sqlString.Substring(indx, (closeCommentIdx- indx)+2));
indx = closeCommentIdx + 1;
continue;
Expand Down Expand Up @@ -112,7 +113,7 @@ public static void Parse(string sqlString, IRecognizer recognizer)
if (c == ':')
{
// named parameter
int right = StringHelper.FirstIndexOfChar(sqlString, ParserHelper.HqlSeparators, indx + 1);
int right = StringHelper.FirstIndexOfChar(sqlString, ParserHelper.HqlSeparatorsAsCharArray, indx + 1);
int chopLocation = right < 0 ? sqlString.Length : right;
string param = sqlString.Substring(indx + 1, chopLocation - (indx + 1));
recognizer.NamedParameter(param, indx);
Expand All @@ -124,7 +125,7 @@ public static void Parse(string sqlString, IRecognizer recognizer)
if (indx < stringLength - 1 && char.IsDigit(sqlString[indx + 1]))
{
// a peek ahead showed this as an ejb3-positional parameter
int right = StringHelper.FirstIndexOfChar(sqlString, ParserHelper.HqlSeparators, indx + 1);
int right = StringHelper.FirstIndexOfChar(sqlString, ParserHelper.HqlSeparatorsAsCharArray, indx + 1);
int chopLocation = right < 0 ? sqlString.Length : right;
string param = sqlString.Substring(indx + 1, chopLocation - (indx + 1));
// make sure this "name" is an integral
Expand Down Expand Up @@ -160,4 +161,4 @@ public static void Parse(string sqlString, IRecognizer recognizer)
}
}
}
}
}
4 changes: 2 additions & 2 deletions src/NHibernate/Hql/Ast/ANTLR/Util/JoinProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,12 @@ public static void ProcessDynamicFilterParameters(

private static bool HasDynamicFilterParam(SqlString sqlFragment)
{
return sqlFragment.IndexOfCaseInsensitive(ParserHelper.HqlVariablePrefix) < 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't understand this code. Why "has dynamic filter param" is true when HqlVariablePrefix is missing?

return !ParserHelper.HasHqlVariable(sqlFragment);
}

private static bool HasCollectionFilterParam(SqlString sqlFragment)
{
return sqlFragment.IndexOfCaseInsensitive("?") < 0;
return sqlFragment.IndexOfOrdinal("?") < 0;
}

private class JoinSequenceSelector : JoinSequence.ISelector
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Hql/Ast/ANTLR/Util/SyntheticAndFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public void AddWhereFragment(
if (hqlSqlWalker.IsFilter())
{
//if (whereFragment.IndexOfCaseInsensitive("?") >= 0)
if (whereFragment.ToString().IndexOf("?") >= 0)
if (whereFragment.IndexOfOrdinal("?") >= 0)
{
IType collectionFilterKeyType = hqlSqlWalker.SessionFactoryHelper
.RequireQueryableCollection(hqlSqlWalker.CollectionFilterRole)
Expand Down
23 changes: 21 additions & 2 deletions src/NHibernate/Hql/ParserHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
using System;
using NHibernate.SqlCommand;

namespace NHibernate.Hql
{
public static class ParserHelper
{
public const string HqlVariablePrefix = ":";

public const string HqlSeparators = " \n\r\f\t,()=<>&|+-=/*'^![]#~\\;";
internal static readonly char[] HqlSeparatorsAsCharArray = HqlSeparators.ToCharArray();
//NOTICE: no " or . since they are part of (compound) identifiers

public const string Whitespace = " \n\r\f\t";
Expand All @@ -13,7 +17,22 @@ public static class ParserHelper

public static bool IsWhitespace(string str)
{
return Whitespace.IndexOf(str) > - 1;
return Whitespace.IndexOf(str, StringComparison.Ordinal) > - 1;
}

internal static bool HasHqlVariable(string value)
{
return value.IndexOf(HqlVariablePrefix, StringComparison.Ordinal) >= 0;
}

internal static bool HasHqlVariable(SqlString value)
{
return value.IndexOfOrdinal(HqlVariablePrefix) >= 0;
}

internal static bool IsHqlVariable(string value)
{
return value.StartsWith(HqlVariablePrefix, StringComparison.Ordinal);
}
}
}
}
2 changes: 1 addition & 1 deletion src/NHibernate/Impl/SessionImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2034,7 +2034,7 @@ public override IDictionary<string, IFilter> EnabledFilters

private string[] ParseFilterParameterName(string filterParameterName)
{
int dot = filterParameterName.IndexOf(".");
int dot = filterParameterName.IndexOf('.');
if (dot <= 0)
{
throw new ArgumentException("Invalid filter-parameter name format", "filterParameterName");
Expand Down
12 changes: 4 additions & 8 deletions src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,9 @@ private string GetWholeAssociationPath(CriteriaImpl.Subcriteria subcriteria)
// some messy, complex stuff here, since createCriteria() can take an
// aliased path, or a path rooted at the creating criteria instance
ICriteria parent = null;
if (path.IndexOf('.') > 0)
if (StringHelper.IsNotRoot(path, out var testAlias))
{
// if it is a compound path
string testAlias = StringHelper.Root(path);
if (!testAlias.Equals(subcriteria.Alias))
{
// and the qualifier is not the alias of this criteria
Expand Down Expand Up @@ -678,9 +677,8 @@ private Persister.Entity.IPropertyMapping GetPropertyMapping(string entityName)

public string GetEntityName(ICriteria subcriteria, string propertyName)
{
if (propertyName.IndexOf('.') > 0)
if (StringHelper.IsNotRoot(propertyName, out var root))
{
string root = StringHelper.Root(propertyName);
ICriteria crit = GetAliasedCriteria(root);
if (crit != null)
{
Expand All @@ -692,9 +690,8 @@ public string GetEntityName(ICriteria subcriteria, string propertyName)

public string GetSQLAlias(ICriteria criteria, string propertyName)
{
if (propertyName.IndexOf('.') > 0)
if (StringHelper.IsNotRoot(propertyName, out var root))
{
string root = StringHelper.Root(propertyName);
ICriteria subcriteria = GetAliasedCriteria(root);
if (subcriteria != null)
{
Expand All @@ -706,9 +703,8 @@ public string GetSQLAlias(ICriteria criteria, string propertyName)

public string GetPropertyName(string propertyName)
{
if (propertyName.IndexOf('.') > 0)
if (StringHelper.IsNotRoot(propertyName, out var root))
{
string root = StringHelper.Root(propertyName);
ICriteria crit = GetAliasedCriteria(root);
if (crit != null)
{
Expand Down
4 changes: 2 additions & 2 deletions src/NHibernate/Loader/Loader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1753,7 +1753,7 @@ protected void AdjustQueryParametersForSubSelectFetching(SqlString filteredSqlSt
protected SqlString ExpandDynamicFilterParameters(SqlString sqlString, ICollection<IParameterSpecification> parameterSpecs, ISessionImplementor session)
{
var enabledFilters = session.EnabledFilters;
if (enabledFilters.Count == 0 || sqlString.ToString().IndexOf(ParserHelper.HqlVariablePrefix) < 0)
if (enabledFilters.Count == 0 || !ParserHelper.HasHqlVariable(sqlString))
{
return sqlString;
}
Expand All @@ -1776,7 +1776,7 @@ protected SqlString ExpandDynamicFilterParameters(SqlString sqlString, ICollecti

foreach (string token in tokens)
{
if (token.StartsWith(ParserHelper.HqlVariablePrefix))
if (ParserHelper.IsHqlVariable(token))
{
string filterParameterName = token.Substring(1);
string[] parts = StringHelper.ParseFilterParameterName(filterParameterName);
Expand Down
5 changes: 5 additions & 0 deletions src/NHibernate/SqlCommand/SqlString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,11 @@ public int IndexOfCaseInsensitive(string text)
return IndexOf(text, 0, _length, StringComparison.InvariantCultureIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to me this one should be switched to StringComparison.OrdinalIgnoreCase. Invariant does some Unicode character equivalency logic, which is most likely to cause "false positives" from a query standpoint. Moreover case insensitive ordinal is more performant than invariant. And this method is used either on ASCII characters or on query aliases.

But overall they are more than hundred cases were invariant is used in the code base. It looks to me most of them if not all should be switched to ordinal. Maybe worth a dedicated PR.

(By the way if changing this case here, the text parameter comment looks obsolete, its last part should be removed.)

Copy link
Member Author

@bahusoid bahusoid Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to me this one should be switched to StringComparison.OrdinalIgnoreCase.

Yeah. But it's public method. I'm afraid to change its behaviour :)

It looks to me most of them if not all should be switched to ordinal.

Agreed (but not as part of this PR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. But it's public method. I'm afraid to change its behaviour :)

Fixing a behavior which is a bug is OK. I do not think the invariant behavior discrepancies with ordinal can be anything else than bugs for this case, but well, maybe I am wrong. Something that is equal with invariant but not equal with ordinal looks very likely to me to be considered not equal by the database query parser as well, in which case we would have a bug. Still this can be the subject of another PR, switching all invariant which should be to ordinal. I also think the change of most of them could be considered as also fixing bug possibilities.

@hazzik, what is your view on this subject? Do you think it is ok to switch from invariant to ordinal in a minor release, considering it is also a bug fix, or would you rather have such change in a major release?

}

internal int IndexOfOrdinal(string text)
{
return IndexOf(text, 0, _length, StringComparison.Ordinal);
}

/// <summary>
/// Returns the index of the first occurrence of <paramref name="text" />, case-insensitive.
/// </summary>
Expand Down
Loading