Avoid unnecessary persister lookup in Loader

[bahusoid]

Additional persister lookup is really required only for subclasses (rootPersister.HasSubclasses && rootPersister.EntityName != instanceClass, also see GetInstanceClass where instanceClass is retrieved)

But let's simply check if given entity name comes directly from provided persister.

[hazzik]

Are you sure that should be ReferenceEquals?

[bahusoid]

I think it's pretty safe and most effective.

I've verified that the same instance is retrieved when subclass lookup persister.GetSubclassForDiscriminatorValue is performed for base subclass discriminator value. And from what I see all entity names come from PersistentClass.EntityName

Maybe I should add an additional comment to GetInstanceClass. Something like: if entity name is dynamically calculated and might be equal to persister.EntityName please return perster.EntityName to avoid additional persister lookup in GetResultRow.

Or just let me know if you think it's better to use full string comparison here :)

[fredericDelaporte]

Personally I am hard to convince about doing such micro-optimisation. Here we do not actually care about the string having the same reference but about them being the same according to default equality.
Default string equality is already quite optimized. I do not think it is worth trying to be "smarter", at the risk of causing more inefficiency with some later code change, even if "protected" by a comment asking for coding in a special way due to this micro-optimization here.

[bahusoid]

Ok (though I don't think this code can cause more inefficiency than before) What about comparer? :) Ordinal? Default (culture specific)?

I see that SessionFactoryImpl.entityPersisters is created with default culture specific comparer. Is it really right? I think it should be either Ordinal or Invariant (I'm voting for Ordinal for entity names)

[fredericDelaporte]

Invariant is indeed still culture specific. Sure, Invariant is for many technical cases just a bad thing, Ordinal should be preferred over it in most cases. Anyway, I am for a solution using the string.Equals(string, string) "no culture" method, which is called by == operator, and which is almost equivalent in implementation to using Ordinal.

So if you really want to use Ordinal (for the sake of a R# or FxCop warning?), go for it, but otherwise just keep it simple, == would perfectly do the job here.

By the way, where do you see

SessionFactoryImpl.entityPersisters is created with default culture specific comparer

?

I see entityPersisters = new Dictionary<string, IEntityPersister>();, which means use of default comparer (EqualityComparer<T>.Default), which uses IEquatable implementation when available, which in the case of string is implemented like string.Equals(string, string), without any notion of culture.

[bahusoid]

Oups.. For some reasons I thought that string.Equals does by default culture specific comparison.. Sorry for all the fuss :)

[hazzik]

@bahusoid put of curiosity: are there any measurable benefits of doing this change?

[bahusoid]

Well this lookup was executed for each row and for each entity in row. So the benefit is really configuration/data specific. It really should be noticeable if you load big amount of data and have many persisters in configuration and no subclasses in query.

Actually I've noticed another place to improve here:

nhibernate-core/src/NHibernate/Loader/Loader.cs

Lines 1079 to 1082 in bea25bf

	// This is not very nice (and quite slow):
	string[][] cols = persister == rootPersister
	? EntityAliases[i].SuffixedPropertyAliases
	: EntityAliases[i].GetSuffixedPropertyAliases(persister);

For subclass persister column aliases are recalculated for each row - EntityAliases[i].GetSuffixedPropertyAliases(persister). We can cache them by i, entityName.
Do you agree? I can do it as part of this PR.

[fredericDelaporte]

and have many persisters in configuration

Why? This is a dictionary lookup, so an O(1) operation, not an O(n). Cardinality of persisters has theoretically no impact (it may still have some in case of bad hashcode distribution).

If this change actually gives a performance benefit, I think it should be noticeable in the "RawDataAccessBencher" benchmark.

For subclass persister column aliases are recalculated for each row - EntityAliases[i].GetSuffixedPropertyAliases(persister). We can cache them by i, entityName.

Makes sens. (entityName being here hold in instanceClass local variable.)

I can do it as part of this PR.

Better as another one I think.

[bahusoid]

It really should be noticeable

I've made some tests... And no - it won't be visibly noticeable in most cases. But on 100,000 iterations I clearly see a difference 1ms vs 5-7 ms :) Still - little bit here and little bit there and it will make a difference on the whole :)

[fredericDelaporte]

So you may be interested in having a look into #476, it is also a "perf PR", stale for some time.

[bahusoid]

Well interface changes.. No I'm not into it..