Skip to content
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.

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

EO Objects can have the same hash in the new version #2157

Closed
levBagryansky opened this issue Jun 16, 2023 · 14 comments · Fixed by #2167
Closed

EO Objects can have the same hash in the new version #2157

levBagryansky opened this issue Jun 16, 2023 · 14 comments · Fixed by #2167
Assignees
Labels

Comments

@levBagryansky
Copy link
Member

This test is slight modification from eo-threads:

    @Test
    public void phiUniqueHashesInDynamic() {
        final int threads = 1500;
        final Set<Integer> hashes = ConcurrentHashMap.newKeySet();
        new Threads<>(
            threads,
            Stream.generate(
                () -> (Scalar<Integer>) () -> (new EOstring$EOlength(Phi.Φ)).hashCode()
            ).limit(threads).collect(Collectors.toList())
        ).forEach(hashes::add);
        MatcherAssert.assertThat(
            hashes.size(),
            Matchers.equalTo(threads)
        );
    }

The test guaranteed that objects created in parallel have different hashcodes. The test does not passes in new version anymore so eo-threads does not work correctly(objectionary/eo-threads#87).
There is similar test in this repo (https://github.com/objectionary/eo/blob/master/eo-runtime/src/test/java/org/eolang/VerticesTest.java#L70) and it passes. Also the test below:

@Test
    public void phiUniqueHashesSeq() {
        final int objects = 1500;
        final Set<Integer> hashes = ConcurrentHashMap.newKeySet();
        for (int i = 0; i < objects; i++) {
            hashes.add((new EOstring$EOlength(Phi.Φ)).hashCode());
        }
        MatcherAssert.assertThat(
            hashes.size(),
            Matchers.equalTo(objects)
        );
    }

passes too.
@volodya-lombrozo please take a look.

@levBagryansky levBagryansky changed the title Objects can have the same hash in the new version EO Objects can have the same hash in the new version Jun 16, 2023
@volodya-lombrozo
Copy link
Member

@levBagryansky Looks like it is expected behaviour. The hashCode is actually the PhDefault.vertex.
Since we have protected static final ThreadLocal<Vertices> VTX = ThreadLocal.withInitial(Vertices::new); which is thread-local for now, it is expected that in each thread we have the same vertex number. Code from PhDefault:

protected static final ThreadLocal<Vertices> VTX = ThreadLocal.withInitial(Vertices::new);

public PhDefault(final Phi sigma) {
    //... 
    this.vertex = PhDefault.VTX.get().next();
    //...
}

@Override
public int hashCode() {
    return this.vertex;
}

Why do you need to check this?

The test guaranteed that objects created in parallel have different hashcodes

@levBagryansky
Copy link
Member Author

@volodya-lombrozo We need it to distinguish objects in eo-threads.

@volodya-lombrozo
Copy link
Member

@levBagryansky I'm afraid that it's dangerous, because:

It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results

https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html

In other words, If two objects are unequal, their hash code are not required to be different.

@levBagryansky
Copy link
Member Author

@volodya-lombrozo You are right. But In eo-threads and in for example Heaps.java uses PhDefault.equals() method to distinguish eo objects in the map.

@Override
    public boolean equals(final Object obj) {
        return obj instanceof Phi && this.hashCode() == obj.hashCode();
    }

That is why it is important to assign different hashcodes to different eo objects.

@volodya-lombrozo
Copy link
Member

@levBagryansky Maybe we have to fix equals method either?

@levBagryansky
Copy link
Member Author

@volodya-lombrozo I think it would help too

@volodya-lombrozo
Copy link
Member

@levBagryansky Could you help us here?

@levBagryansky
Copy link
Member Author

@volodya-lombrozo sure

@levBagryansky
Copy link
Member Author

levBagryansky commented Jun 19, 2023

@volodya-lombrozo I think we need to fix vertex field instead.

    /**
     * Identity of it (the ID of the vertex).
     * @checkstyle VisibilityModifierCheck (2 lines)
     */
    protected int vertex;

Now it cannot be "the ID of the vertex" since the can be the same in different threads.
Its right to implenent equals via vertex then.

@volodya-lombrozo
Copy link
Member

volodya-lombrozo commented Jun 19, 2023

@levBagryansky
You can try to remove ThreadLocal and it will work, I hope, but I'm not sure that it will be correct solution.

levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
levBagryansky added a commit to levBagryansky/eo that referenced this issue Jun 19, 2023
@levBagryansky
Copy link
Member Author

@yegor256 can we release these changes in order to fix eo-threads?

@yegor256
Copy link
Member

yegor256 commented Jul 3, 2023

@rultor release, tag is 0.29.6

@rultor
Copy link
Contributor

rultor commented Jul 3, 2023

@rultor release, tag is 0.29.6

@yegor256 OK, I will release it now. Please check the progress here

@rultor
Copy link
Contributor

rultor commented Jul 3, 2023

@rultor release, tag is 0.29.6

@yegor256 Done! FYI, the full log is here (took me 17min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants