Refactored DefaultEntityAliases to avoid unnecessary calculations #1482

bahusoid · 2017-12-11T06:00:39Z

DefaultEntityAliases is actively used for generating column aliases for entities in T-SQL SELECT statements.

My refactorings:

Avoided default alias calculation if user provided alias is present;
Added early exits if no user provided aliases exist (to avoid lookup for all columns on empty dic);
Made RowIdAlias lazy (other fields are used one way or another but this one is dialect specific);
Removed string.IsIntern calls for each generated alias. I didn't find any places where aliases can be interned inside NHibernate project (and can prepare a pull request that shows that string.IsIntern for aliases always returns null for all tests). According to b1500ea string.IsIntern was introduced instead of string.Intern to avoid excessive memory usage. But I don't understand why NHibernate should expect interned aliases - string.IsIntern doesn't look right to me here.

fredericDelaporte · 2017-12-11T10:52:28Z

About intern string, they seem to be there as a result of the Java port, see NH-902 comments. (That is the ticket bound to the commit you have linked.)

About interns - do you have a special reason to use them in C#/CLR? Or is it a Java legacy?

Yes, interns are there because they were there in Java.

I implemented sort of a compromise - string.IsInterned will be used to find whether there is an equivalent interned string, and that one will be returned if found. If not, the original string will be used.

So initially they were always interned, not just switched to their intern version if one was there. But apparently reading from NH-902, this was causing high memory consumption due to interning to many various strings, which otherwise would have been garbage collected.

The applied solution to NH-902 does not look to me good. From a situation where too many various strings were interned we are now in a situation where maybe commonly used strings are not interned. That is not a domain where a systematic approach works. Interning should be handled on a case by case basis.

In the case of aliases, can we expect aliases to use only a small set of string values? If yes, they should be interned (reverting NH-902 for them). If no, if instead they have a large set of potential values, no interning should be handled, as your PR do.

If the situation is mixed, meaning if there is a set of common values frequently used along with a large set of various values, then NH-902 solution could have been beneficial if something had been added for interning the set of common values. But that is not the case according to your findings.

I think the safer is to remove interning handling for them. There is a suffix logic for avoiding aliases conflict which causes them to be numerous, so better not revert NH-902 for them.

Ideally it seems to me we should check all the other interning to check if they make sens.

bahusoid · 2017-12-11T13:30:05Z

@fredericDelaporte I agree - the whole b1500ea looks suspicious. Interning might be useful for long living objects such as various string metadata in session factory (so commonly used property/column names and other string metadata between multiple factories are shared).

But IMHO it should not be used in frequently called methods - DefaultEntityAliases instances are created for each query executions. And it's not long living objects (it's scoped to Loader life) - we already generated alias (memory is occupied) and obtaining interned alias here is just a waste of CPU/memory.

fredericDelaporte · 2017-12-11T13:41:16Z

Yes, I think removing interning for aliases is right. For other cases, we may consider opening other issues or PR.

fredericDelaporte · 2017-12-11T16:58:08Z

src/NHibernate/Loader/DefaultEntityAliases.cs

-				suffixedKeyColumns = keyColumnsCandidates;
-			}
-			Intern(suffixedKeyColumns);
+			suffixedKeyColumns = GetSuffixedKeyAliases(persister, suffix);


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.

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

hazzik · 2017-12-12T22:17:06Z

src/NHibernate/Loader/DefaultEntityAliases.cs

 			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));


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

@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

Yeah, it seems that you're right.

hazzik · 2017-12-13T00:04:02Z

@bahusoid I still would like this to be refactored without using lambdas.

bahusoid · 2017-12-13T03:30:21Z

@hazzik What's wrong with lambdas? I can get rid of the "access to the modified closure" and still use lambdas. I don't understand your concerns - could you please be more specific. How do you prefer to see "deferred value calculation" in some method.

hazzik · 2017-12-13T03:38:01Z

There is no need to have “deferred” calculations. It’s all nice and clean with just simple ??. Just need to inline some of the private methods and reassemble them into other methods.

I actually did the refactoring, but wanted to give you a credit for the idea and the work.

bahusoid · 2017-12-13T03:49:19Z

Ah.. I see :) Ok. I can do it

bahusoid · 2017-12-13T04:05:31Z

@hazzik Done

fredericDelaporte · 2017-12-13T08:09:35Z

There is no need to have “deferred” calculations. It’s all nice and clean with just simple ??. Just need to inline some of the private methods and reassemble them into other methods.

Indeed, I should have thought about it.

hazzik · 2017-12-13T11:51:26Z

I've allowed myself to cleanup the code. Please review. @bahusoid @fredericDelaporte

fredericDelaporte

Some shortcuts are removed but it is more clean, and those shortcuts were only quite small savings.

fredericDelaporte · 2017-12-13T12:16:33Z

src/NHibernate/Loader/DefaultEntityAliases.cs

-			}
-			else
+			string[] columns = GetUserProvidedAlias(AbstractEntityPersister.EntityClass);
+			if (columns != null)


The last refactoring removed by the way a discrepancy in previous refactoring causing a null value as first columns value to be replaced. So the last refactoring fix an undue change. (That said, this likely concerns an error case.)

I don't get it.

The previous state of the PR was somewhat equivalent to

return columns?[0] ?? GetDiscriminatorAlias(persister, _suffix);

While the master code is more like current state.

So on master, if columns[0] contains null, null is yielded as an alias (which is likely an error case). With your last commit it will also be the case. But without your last commit, it would have been replaced by GetDiscriminatorAlias(persister, _suffix).

I believe that this is a correct behaviour: if we got null in columns[0] we should not swallow this case, but fail somewhere.

Yes, agreed.

bahusoid · 2017-12-13T13:04:23Z

@hazzik My IMHOs:

Now it's really hard to understand what's been changed in this PR. I personally think that fields renaming and method orderings should be commited as separate changeset without others changes.
@fredericDelaporte

and those shortcuts were only quite small savings

Maybe. But the thing is - I believe that user provided aliases are hardly ever used by most users. So those shortcuts are the main route of code for most users - why not to save a little? See example of test with custom provided aliases - NativeSqlCollectionLoaderFixture.LoadCompositeElementsWithWithCustomAliases

hazzik · 2017-12-13T21:04:56Z

@bahusoid do you want me to redo this in 2 commits? I don't mind.

bahusoid · 2017-12-14T05:10:32Z

@hazzik Yeah it would be better. And I want my shortcuts back :)

hazzik · 2017-12-14T05:26:53Z

hazzik · 2017-12-14T22:12:55Z

@bahusoid done.

bahusoid · 2017-12-15T03:00:51Z

@hazzik Have nothing to complain about.

Though... )) Do we really need both SafeGetUserProvidedAliases/GetUserProvidedAliases? "Unsafe" version is used only twice and only if user aliases are provided. I think code would be cleaner with only safe version.

Just a thought - it's fine by me to keep it.

fredericDelaporte · 2017-12-15T10:12:27Z

Though... )) Do we really need both SafeGetUserProvidedAliases/GetUserProvidedAliases? "Unsafe" version is used only twice and only if user aliases are provided. I think code would be cleaner with only safe version.

This is always a tricky part with shortcuts/small code optimizations: up to which point should we do them? That is why I try to not debate much on them, unless some benchmark show concrete effects.

About the merge, I was thinking the point of splitting the commits was not only for review, but also for helping understanding the changes in the history. So merging them all squashed to a single commit was not what I was expecting.
Of course it would have required to have this PR commits squashed in a minimal set of commits doing only the final changes but still split between "code renamed, restyled and moved around" and "code logic changed".

fredericDelaporte · 2018-06-05T14:34:05Z

src/NHibernate/Loader/DefaultEntityAliases.cs

+		public string[] SuffixedKeyAliases { get; }
+
+		// TODO: not visible to the user!
+		public string RowIdAlias => _rowIdAlias ?? (_rowIdAlias = Loadable.RowIdAlias + _suffix);


I think none of us have realized we were changing a class which instances can be used by concurrent threads. In a concurrent usage situation, this micro-optimization can instead be a micro-performance loss! We would have better not have done it in my opinion.

bahusoid force-pushed the defAliases branch from 989d5af to 4dd8349 Compare December 11, 2017 06:08

Refactored DefaultEntityAliases to avoid unnecessary calculations

d0006b7

bahusoid force-pushed the defAliases branch from 4dd8349 to d0006b7 Compare December 11, 2017 06:10

fredericDelaporte self-requested a review December 11, 2017 11:03

fredericDelaporte reviewed Dec 11, 2017

View reviewed changes

Structured changes closer to Hibernate version; Added few comments

0d613de

fredericDelaporte previously approved these changes Dec 12, 2017

View reviewed changes

fredericDelaporte self-assigned this Dec 12, 2017

fredericDelaporte added this to the 5.1 milestone Dec 12, 2017

fredericDelaporte added c: Core p: Minor t: Improvement labels Dec 12, 2017

hazzik previously requested changes Dec 12, 2017

View reviewed changes

hazzik added p: Lowest and removed p: Minor labels Dec 12, 2017

bahusoid dismissed fredericDelaporte’s stale review via 6c728e6 December 13, 2017 03:59

bahusoid force-pushed the defAliases branch from 6c728e6 to 9da7cd9 Compare December 13, 2017 04:18

Get rid of delegates

e1ce4b6

bahusoid force-pushed the defAliases branch from 9da7cd9 to e1ce4b6 Compare December 13, 2017 06:43

fredericDelaporte previously approved these changes Dec 13, 2017

View reviewed changes

hazzik dismissed fredericDelaporte’s stale review via 71bfe8e December 13, 2017 11:47

hazzik force-pushed the defAliases branch from 71bfe8e to 0c81cd7 Compare December 13, 2017 11:49

fredericDelaporte previously approved these changes Dec 13, 2017

View reviewed changes

hazzik added 5 commits December 15, 2017 09:50

Merge branch 'master' into bahusoid-defAliases

4be3ff9

Code cleanup

a48c9e3

Reuse _suffix field instead of passing it around

04d12e2

Refactoring

240f1e2

Reformat code

32c946c

hazzik dismissed fredericDelaporte’s stale review via 32c946c December 14, 2017 22:10

hazzik force-pushed the defAliases branch from 9827d90 to 32c946c Compare December 14, 2017 22:10

hazzik approved these changes Dec 15, 2017

View reviewed changes

hazzik merged commit aada800 into nhibernate:master Dec 15, 2017

fredericDelaporte added t: Fix r: Fixed and removed t: Fix labels Dec 15, 2017

bahusoid mentioned this pull request Dec 23, 2017

[WIP] Intern string metadata in Session Factory #1503

Closed

fredericDelaporte reviewed Jun 5, 2018

View reviewed changes

Refactored DefaultEntityAliases to avoid unnecessary calculations #1482

Refactored DefaultEntityAliases to avoid unnecessary calculations #1482

Uh oh!

Conversation

bahusoid commented Dec 11, 2017

Uh oh!

fredericDelaporte commented Dec 11, 2017

Uh oh!

bahusoid commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Dec 11, 2017

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!

bahusoid Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Dec 13, 2017

Uh oh!

bahusoid commented Dec 13, 2017

Uh oh!

hazzik commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Dec 13, 2017

Uh oh!

bahusoid commented Dec 13, 2017

Uh oh!

fredericDelaporte commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Dec 13, 2017

Uh oh!

fredericDelaporte 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!

fredericDelaporte Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bahusoid commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Dec 13, 2017

Uh oh!

bahusoid commented Dec 14, 2017

Uh oh!

hazzik commented Dec 14, 2017

Uh oh!

hazzik commented Dec 14, 2017

Uh oh!

bahusoid commented Dec 15, 2017

Uh oh!

fredericDelaporte commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bahusoid commented Dec 11, 2017 •

edited

Loading

bahusoid Dec 12, 2017 •

edited

Loading

hazzik commented Dec 13, 2017 •

edited

Loading

fredericDelaporte commented Dec 13, 2017 •

edited

Loading

fredericDelaporte Dec 13, 2017 •

edited

Loading

bahusoid commented Dec 13, 2017 •

edited

Loading

fredericDelaporte commented Dec 15, 2017 •

edited

Loading