Fixing allPropertiesDo duplications v2

[AntoninGoslin]

Following up the PR #119
I made a second version of the fix for duplication of properties.
Fixed this problem by putting back the original pattern to keep optimization, and adding a method. allPropertiesDo: block visited: aSet to filter the properties duplications that we encounter thanks to a set.
Is this version better @Gabriel-Darbord @jecisc ?

Closes #118

[Gabriel-Darbord]

Thanks, that looks better :)
Though, why did you make a new PR instead of updating #119?

Also, have you considered using an IdentitySet? I'm not asking for a change, but it's usually faster and shows clearer intent.

[AntoninGoslin]

I just wanted to keep the old version safe, i'm still a newbie using GH, sorry :) .
Concerning IdentitySet I don't really see the difference with a Set. Is that better because we could avoid to use strings ?

[Gabriel-Darbord]

IdentitySet and IdentityDictionary use identity (==) instead of equality (=).
Identity between two references means they point to the exact same object in memory. It is never overridden.
Equality is way less strict, it allows for any two objects to be equal as long as the logic implemented in the equality method says so. It can be overridden, but defaults to identity. So unless you need equality logic, it's faster and more straightforward to use identity.
When using Strings, it can be tricky because copies are not identical, but Symbols are always unique so there's no risk there:

'foo' == 'foo'. "true"
'foo' == 'foo' copy. "false"
#foo == #foo copy. "true"

I'm not sure if it can work without relying on the name, you'll have to try it if you're curious.

[AntoninGoslin]

I will search how I can use IdentitySet, thanks you.

[AntoninGoslin]

Note to myself, we also need to fix the case where a property is in a superclass and a trait

[Gabriel-Darbord]

Tests are failing because FamixJavaClass does not exist in the Fame repository.
You should reuse or add test doubles.

[jecisc]

So here we have two problems. The tests are depending on Famix and also the behavior of #allProperties changed.

I explained to antonin how to do tests without using Famix. Also he will update the PR to use once again its previous implementation that worked, and he will add a cache on #allProperties so that we do not have perfomance regression on model import

[AntoninGoslin]

I pushed a third version using a cache.
Here is the benchmark and the recap:

desc := FamixJavaClass mooseDescription.

"Version 1: allProperties defined in terms of allPropertiesDo:"
[desc allProperties1 ] bench. "12,707 iterations in 5 seconds 1 millisecond. 2540.892 per second"
[desc allPropertiesDo1: [ :prop | "nothing" ] ] bench. "13,000 iterations in 5 seconds 2 milliseconds. 2598.960 per second"

"Version 2: allPropertiesDo: defined in terms of allProperties, remove duplicates with internal set"
[desc allProperties2 ] bench.  "170,219 iterations in 5 seconds 2 milliseconds. 34030.188 per second"
[desc allPropertiesDo2: [ :prop | "nothing" ] ] bench. "191,604 iterations in 5 seconds. 38320.800 per second"

"Version 3: allProperties defined in terms of allPropertiesDo:, closer to Version 1, with cache"
[desc allProperties3 ] bench.  "449,072,055 iterations in 5 seconds. 89814411.000 per second"
[desc allPropertiesDo3: [ :prop | "nothing" ] ] bench. "7,616,784 iterations in 4 seconds 999 milliseconds. 1523661.532 per second"

• The V1 is the version using the inverted pattern by building the dictionnary is allPropertiesand iterate it on allPropertiesDo
• The V2 is bringing back orginal pattern using a set to counter doubles
• The V3 re use the V1, with a cache and less code duplication

Our V3 is 35  000 times faster than the V1, and around 2500 times faster than the V2.

[Gabriel-Darbord]

Caching is obviously faster, but uses more space.
Be careful when benchmarking in future: you should also consider the cost of the additional storage required.
This is fine in this case, thanks for the PR :)