Skip to content

Refactored DefaultEntityAliases to avoid unnecessary calculations #1482

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 8 commits into from
Dec 15, 2017
Merged

Refactored DefaultEntityAliases to avoid unnecessary calculations #1482

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d0006b7
Refactored DefaultEntityAliases to avoid unnecessary calculations
bahusoid Dec 11, 2017
0d613de
Structured changes closer to Hibernate version; Added few comments
bahusoid Dec 12, 2017
e1ce4b6
Get rid of delegates
bahusoid Dec 13, 2017
4be3ff9
Merge branch 'master' into bahusoid-defAliases
hazzik Dec 14, 2017
a48c9e3
Code cleanup
hazzik Dec 14, 2017
04d12e2
Reuse _suffix field instead of passing it around
hazzik Dec 14, 2017
240f1e2
Refactoring
hazzik Dec 14, 2017
32c946c
Reformat code
hazzik Dec 14, 2017
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
85 changes: 45 additions & 40 deletions src/NHibernate/Loader/DefaultEntityAliases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using NHibernate.Persister.Entity;
using NHibernate.Util;

namespace NHibernate.Loader
{
Expand All @@ -15,43 +16,26 @@ public class DefaultEntityAliases : IEntityAliases
private readonly string[][] suffixedPropertyColumns;
private readonly string suffixedDiscriminatorColumn;
private readonly string suffix;
private readonly string rowIdAlias;
private string rowIdAlias;
private readonly IDictionary<string, string[]> userProvidedAliases;

public DefaultEntityAliases(ILoadable persister, string suffix)
: this(CollectionHelper.EmptyDictionary<string, string[]>(), persister, suffix) {}
: this(null, persister, suffix){}

/// <summary>
/// Calculate and cache select-clause suffixes.
/// </summary>
public DefaultEntityAliases(IDictionary<string, string[]> userProvidedAliases, ILoadable persister, string suffix)
{
this.suffix = suffix;
this.userProvidedAliases = userProvidedAliases;
this.userProvidedAliases = userProvidedAliases?.Count > 0 ? userProvidedAliases : null;

string[] keyColumnsCandidates = GetUserProvidedAliases(persister.IdentifierPropertyName, null);
if (keyColumnsCandidates == null)
{
suffixedKeyColumns = GetUserProvidedAliases(EntityPersister.EntityID, GetIdentifierAliases(persister, suffix));
}
else
{
suffixedKeyColumns = keyColumnsCandidates;
}
Intern(suffixedKeyColumns);
suffixedKeyColumns = GetSuffixedKeyAliases(persister, suffix);
Copy link
Member

Choose a reason for hiding this comment

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

This new GetSuffixedKeyAliases has also been introduced on Hibernate side, under the name determineKeyAlias. Better use Hibernate name and try to order things similarly, for easing comparing the state of the two. (But adjust casing to .Net conventions of course, so DetermineKeyAlias.) See https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/loader/DefaultEntityAliases.java

Now on their side they have not optimized the generated indexes and they have introduced other changes which should likely not be taken here (like the unquote thing), so your PR code will of course have differences.

If renaming, the other GetSuffixed methods already there should be renamed too in order to better match Hibernate code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fredericDelaporte Done. Let me know if I miss something.


suffixedPropertyColumns = GetSuffixedPropertyAliases(persister);
suffixedDiscriminatorColumn =
GetUserProvidedAlias(AbstractEntityPersister.EntityClass, GetDiscriminatorAlias(persister, suffix));
if (persister.IsVersioned)
{
suffixedVersionColumn = suffixedPropertyColumns[persister.VersionProperty];
}
else
{
suffixedVersionColumn = null;
}
rowIdAlias = Loadable.RowIdAlias + suffix; // TODO: not visible to the user!
suffixedDiscriminatorColumn = GetSuffixedDiscriminatorAlias(persister, suffix);

suffixedVersionColumn = persister.IsVersioned ? suffixedPropertyColumns[persister.VersionProperty] : null;
}

protected virtual string GetDiscriminatorAlias(ILoadable persister, string suffix)
Expand All @@ -69,12 +53,20 @@ protected virtual string[] GetPropertyAliases(ILoadable persister, int j)
return persister.GetPropertyAliases(suffix, j);
}

private string[] GetUserProvidedAliases(string propertyPath, string[] defaultAliases)
/// <summary>
/// Returns default aliases for all the properties
/// </summary>
protected string[][] GetPropertiesAliases(ILoadable persister)
{
return Enumerable.Range(0, persister.PropertyNames.Length).Select(i => GetPropertyAliases(persister, i)).ToArray();
}

private string[] GetUserProvidedAliases(string propertyPath, Func<string[]> getDefaultAliases)
{
string[] result = propertyPath == null ? null : GetUserProvidedAlias(propertyPath);
if (result == null)
{
return defaultAliases;
return getDefaultAliases();
}
else
{
Expand All @@ -89,27 +81,47 @@ private string[] GetUserProvidedAlias(string propertyPath)
return result;
}

private string GetUserProvidedAlias(string propertyPath, string defaultAlias)
private string GetUserProvidedAlias(string propertyPath, Func<string> getDefaultAlias)
{
string[] columns = propertyPath == null ? null : GetUserProvidedAlias(propertyPath);
if (columns == null)
{
return defaultAlias;
return getDefaultAlias();
}
else
{
return columns[0];
}
}

private string GetSuffixedDiscriminatorAlias(ILoadable persister, string suffix)
{
if (userProvidedAliases == null)
return GetDiscriminatorAlias(persister, suffix);

return GetUserProvidedAlias(AbstractEntityPersister.EntityClass, () => GetDiscriminatorAlias(persister, suffix));
}

private string[] GetSuffixedKeyAliases(ILoadable persister, string suffix)
{
if (userProvidedAliases == null)
return GetIdentifierAliases(persister, suffix);

return GetUserProvidedAliases(
persister.IdentifierPropertyName,
() => GetUserProvidedAliases(EntityPersister.EntityID, () => GetIdentifierAliases(persister, suffix)));
}

public string[][] GetSuffixedPropertyAliases(ILoadable persister)
{
if (userProvidedAliases == null)
return GetPropertiesAliases(persister);

int size = persister.PropertyNames.Length;
string[][] suffixedPropertyAliases = new string[size][];
for (int j = 0; j < size; j++)
{
suffixedPropertyAliases[j] = GetUserProvidedAliases(persister.PropertyNames[j], GetPropertyAliases(persister, j));
Intern(suffixedPropertyAliases[j]);
suffixedPropertyAliases[j] = GetUserProvidedAliases(persister.PropertyNames[j], () => GetPropertyAliases(persister, j));
Copy link
Member

Choose a reason for hiding this comment

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

The () => GetPropertyAliases(persister, j) part is incorrect. When the lambda is resolved j would always be equal to size. See here: https://dotnetfiddle.net/wv8xC1

Copy link
Member Author

@bahusoid bahusoid Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@hazzik I don't think that your example is equal to mine. This lambda is resolved right inside cycle with closured variable and so it's correctly resolved. And in you example you execute action after cycle with modified closured variable. So equivalent would be:
https://dotnetfiddle.net/VrjF80

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it seems that you're right.

}
return suffixedPropertyAliases;
}
Expand All @@ -136,15 +148,8 @@ public string[] SuffixedKeyAliases

public string RowIdAlias
{
get { return rowIdAlias; }
}

private static void Intern(string[] strings)
{
for (int i = 0; i < strings.Length; i++)
{
strings[i] = StringHelper.InternedIfPossible(strings[i]);
}
// TODO: not visible to the user!
get { return rowIdAlias ?? (rowIdAlias = Loadable.RowIdAlias + suffix); }
}
}
}