Hi,

renaming for TMormotHashFactory to TGenericsHashFactory is somehow acceptable (but IMO bad taste) but change for default hashing algorithm in such important library is really really really bad idea.

This was not consulted, change like that or proposition should be reported here :

https://github.com/maciej-izak/generics.collections/issues

OR IN ANY OTHER WAY, not in comment in other bug report where can be easily omitted (!).
The problem with thashmapextendedequalitycomparer example was really minor and should be fixed in other way not by changing *default hashing algorithm* for whole library (!).

Widely used TDictionary<TKey, TValue> is also affected by this change.

Results BEFORE Michael Van Canneyt changes:

THashSet<string> add 2000000 items. Time elapsed : 1844 THashSet<string> lookup 2000000 items. Time elapsed : 812 TSortedHashSet<string> add 2000000 items. Time elapsed : 18640 TSortedHashSet<string> lookup 2000000 items. Time elapsed : 641

THashSet<integer> add 10000000 items. Time elapsed : 1328 THashSet<integer> lookup 10000000 items. Time elapsed : 640 TSortedHashSet<integer> add 10000000 items. Time elapsed : 4235 TSortedHashSet<integer> lookup 10000000 items. Time elapsed : 765

Results AFTER Michael Van Canneyt changes:

THashSet<string> add 2000000 items. Time elapsed : 2250 THashSet<string> lookup 2000000 items. Time elapsed : 906 TSortedHashSet<string> add 2000000 items. Time elapsed : 18828 TSortedHashSet<string> lookup 2000000 items. Time elapsed : 688

THashSet<integer> add 10000000 items. Time elapsed : 2219 THashSet<integer> lookup 10000000 items. Time elapsed : 1140 TSortedHashSet<integer> add 10000000 items. Time elapsed : 5250 TSortedHashSet<integer> lookup 10000000 items. Time elapsed : 1234

As noted in the bugreport, your own examples did not compile with your supplied code. Next time, please test that too.

It is definitely better to have a slower default that works always than a faster one that does not let all code compile.

But not problem, please advise on the changes needed to a) fix the mentioned example so it actually compiles. b) Speed up the default hash.

I (or someone else) will be happy to change it.

Michael.

Maciej Izak schrieb am Sa., 30. Juni 2018, 08:16:

No, the bad taste is too have a class that appears to be named specifically to fit a specific third party project inside a general purpose library.

(though I agree that the change of the default hash could have been discussed first)

Regards,
Sven

2018-06-30 8:33 GMT+02:00 Michael Van Canneyt :

As noted in the bugreport, your own examples did not compile with your supplied code. Next time, please test that too.

The most important was to finish new collections, deep refactoring and proper tests suite than testing all examples. Library was totally refactored and something like that may happens.

It is definitely better to have a slower default that works always than a faster one that does not let all code compile.

No. You are wrong. The more important is test suite than minor example somewhere (which can be easily fixed). For sure faster library with proper test suite is more important than a little outdated example (!sic).

Ok, but in the future you should ask for such important changes.

2018-06-30 8:55 GMT+02:00 Sven Barth via fpc-devel <

Ok, I agree here, but why Michael Van Canneyt decide to annihilating only TmORMotHashFactory and TDelphi*HashFactory still exist? This is highly not consequent!

(though I agree that the change of the default hash could have been discussed first)

seems that compilation for minor example (easy fixable in other way) has bigger priority than major performance of whole library :P

I could do so and I will.
I consider the matter unimportant.

We can argue about this for a long time.
We all have restricted time, so please just provide a correct patch.

Michael.

mORMot is a third party project that can be used with FPC while Delphi is essentially a template and base line for compatibility, so there is a difference between the two. Though if you have a formal name for the hash that Delphi uses we can change that as well :P

Regards,
Sven

Because maybe TDelphi*HashFactory is needed for Delphi compatibility. I did not have time to investigate that.

We do not aspire to compatibility with mORMot, this is for sure, and therefor I renamed it. Feel free to suggest another name.

Michael.

2018-06-30 11:03 GMT+02:00 Sven Barth via fpc-devel <

TDelphi*HashFactory is not required for compatibility with Delphi (as many other things in rtl-generic). Correct name for this hash is "BobJenkins" (Delphi has special slightly modified version of HashLittle which in Delphi is called BobJenkins - don't ask :P). I will fix all hash names where "Delphi" word exist.