Two IR objects aren't the same after de-serialization #10985
Assignees
Labels
Comments
|
According to: it might be a significant failure. If it really means the two deserialized objects aren't equal (at the same location). However I do remember some changes to equality of E.g. this is probably just a result of that |
|
Yes, looks like the culprit of these problems is #10839. To demonstrate try this patch: diff --git lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java lib/java/persistance/src/m
ain/java/org/enso/persist/PerInputImpl.java
index 5bfe457cd2..e57cbbc8c3 100644
--- lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java
+++ lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java
@@ -8,8 +8,10 @@ import java.io.DataInputStream;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
import org.enso.persist.Persistance.Input;
import org.enso.persist.Persistance.Reference;
@@ -202,6 +204,8 @@ final class PerInputImpl implements Input {
return "Input[at=" + at + " of " + this.buf.limit() + "]";
}
+ static final Set<Class<?>> REPORTED = new HashSet<>();
+
static Object readIndirect(InputCache cache, PerMap map, Input in) throws IOException {
var at = in.readInt();
if (at < 0) {
@@ -232,6 +236,22 @@ final class PerInputImpl implements Input {
} else {
throw raise(RuntimeException.class, ex);
}
+ } else {
+ if (!REPORTED.contains(res.getClass())) {
+ var inData2 = new PerInputImpl(cache, at);
+ var res2 = p.readWith(inData2);
+ res2 = cache.resolveObject(res2);
+
+ if (!res.equals(res2)) {
+ if (res.getClass() == res2.getClass()) {
+ PerUtils.LOG.error(res.getClass() + " doesn't support equality properly!");
+ REPORTED.add(res.getClass());
+ } else {
+ throw new IllegalStateException(
+ "Two different classes " + res.getClass() + " vs. " + res2.getClass());
+ }
+ }
+ }
}
return res;
}What the patch does? Immediately after an object is deserialized, another object is deserialized again. They cannot be identical, but they used to be equal. Since #10839 they are not equal anymore. |
|
Here is an output of a program executed with the above patch: e.g. classes from |
|
There are two options to fix the
The simplest way to "fix" diff --git engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
index e0ddd703ae..fdd10289c3 100644
--- engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
+++ engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
@@ -214,7 +214,8 @@ public final class MetadataStorage {
return true;
}
if (obj instanceof MetadataStorage other) {
- return this.metadata == other.metadata;
+ // return this.metadata == other.metadata;
+ return true;
}
return false;
}then the equality problems after deserialization disappear. |
|
I believe the controversial change is https://github.com/enso-org/enso/pull/10839/files#diff-afc9de71a6efb991e62eef6dabe344a48458d77038f21add119e2ebe1deb2919L217 which we somehow relied on quite significantly.
|
A bit hard to be "structurally equal" and still lazy in evaluation, but here is a possible fix: diff --git engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrLazyMap.java engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrLazyMap.java
index 2b1df2fee4..e958be32dc 100644
--- engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrLazyMap.java
+++ engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrLazyMap.java
@@ -11,11 +11,11 @@ import org.enso.persist.Persistance;
import org.enso.persist.Persistance.Reference;
final class IrLazyMap<K, V> extends AbstractMap<K, V> {
- private final Map<K, Entry<K, V>> delegate;
+ private final Map<K, En<K, V>> delegate;
@SuppressWarnings("unchecked")
IrLazyMap(Persistance.Input in) throws IOException {
- var map = new LinkedHashMap<K, Entry<K, V>>();
+ var map = new LinkedHashMap<K, En<K, V>>();
var n = in.readInt();
for (var i = 0; i < n; i++) {
var key = (K) in.readObject();
@@ -37,6 +37,29 @@ final class IrLazyMap<K, V> extends AbstractMap<K, V> {
return entry == null ? null : entry.getValue();
}
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof IrLazyMap otherMap) {
+ var myEntries = delegate.values();
+ var otherEntries = otherMap.delegate.values();
+ if (myEntries.size() != otherEntries.size()) {
+ return false;
+ }
+ var myIt = myEntries.iterator();
+ var otherIt = otherEntries.iterator();
+ while (myIt.hasNext()) {
+ var myElem = myIt.next();
+ var otherElem = otherIt.next();
+ if (!myElem.equals(otherElem)) {
+ return false;
+ }
+ }
+ return true;
+ } else {
+ return super.equals(other);
+ }
+ }
+
private static final class En<K, V> implements Entry<K, V> {
private final K key;
private final Reference<V> ref;
@@ -64,8 +87,9 @@ final class IrLazyMap<K, V> extends AbstractMap<K, V> {
@Override
public int hashCode() {
- int hash = 7;
- hash = 29 * hash + Objects.hashCode(this.key);
+ int hash = 3;
+ hash = 19 * hash + Objects.hashCode(this.key);
+ hash = 19 * hash + Objects.hashCode(this.ref);
return hash;
}
@@ -74,10 +98,17 @@ final class IrLazyMap<K, V> extends AbstractMap<K, V> {
if (this == obj) {
return true;
}
- if (obj instanceof En<?, ?> other) {
- return Objects.equals(this.key, other.key);
+ if (obj == null) {
+ return false;
+ }
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+ final En<?, ?> other = (En<?, ?>) obj;
+ if (!Objects.equals(this.key, other.key)) {
+ return false;
}
- return false;
+ return Objects.equals(this.ref, other.ref);
}
}
}
diff --git engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
index e0ddd703ae..f4ec40e85e 100644
--- engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
+++ engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
@@ -214,7 +214,7 @@ public final class MetadataStorage {
return true;
}
if (obj instanceof MetadataStorage other) {
- return this.metadata == other.metadata;
+ return Objects.equals(this.metadata, other.metadata);
}
return false;
}
diff --git lib/java/persistance/src/main/java/org/enso/persist/PerBufferReference.java lib/java/persistance/src/main/java/org/enso/persist/PerBufferReference.java
index 53d9911114..92676d85e1 100644
--- lib/java/persistance/src/main/java/org/enso/persist/PerBufferReference.java
+++ lib/java/persistance/src/main/java/org/enso/persist/PerBufferReference.java
@@ -1,6 +1,7 @@
package org.enso.persist;
import java.io.IOException;
+import java.util.Objects;
import org.enso.persist.PerInputImpl.InputCache;
import org.enso.persist.Persistance.Reference;
@@ -64,4 +65,34 @@ final class PerBufferReference<T> extends Persistance.Reference<T> {
static <V> Reference<V> cached(Persistance<V> p, InputCache buffer, int offset) {
return new PerBufferReference<>(p, buffer, offset, true);
}
+
+ @Override
+ public int hashCode() {
+ int hash = 7;
+ hash = 67 * hash + Objects.hashCode(this.p);
+ hash = 67 * hash + Objects.hashCode(this.cache);
+ hash = 67 * hash + this.offset;
+ return hash;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null) {
+ return false;
+ }
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+ final PerBufferReference<?> other = (PerBufferReference<?>) obj;
+ if (this.offset != other.offset) {
+ return false;
+ }
+ if (!Objects.equals(this.p, other.p)) {
+ return false;
+ }
+ return Objects.equals(this.cache, other.cache);
+ }
}The basic fix is to make sure that two |
3 tasks
2 tasks
|
|
An alternative approach to the are they equal check is to remove diff --git lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java
index 5bfe457cd2..d9aee8d776 100644
--- lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java
+++ lib/java/persistance/src/main/java/org/enso/persist/PerInputImpl.java
@@ -6,11 +6,16 @@ import static org.enso.persist.PerUtils.raise;
import java.io.DataInputStream;
import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
import java.nio.ByteBuffer;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
+
import org.enso.persist.Persistance.Input;
import org.enso.persist.Persistance.Reference;
@@ -202,6 +207,8 @@ final class PerInputImpl implements Input {
return "Input[at=" + at + " of " + this.buf.limit() + "]";
}
+ static final Set<Class<?>> REPORTED = new HashSet<>();
+
static Object readIndirect(InputCache cache, PerMap map, Input in) throws IOException {
var at = in.readInt();
if (at < 0) {
@@ -219,7 +226,7 @@ final class PerInputImpl implements Input {
res = cache.resolveObject(res);
var prev = cache.putObjectAt(at, res);
if (prev != null) {
- var bothObjectsAreTheSame = Objects.equals(res, prev);
+ var bothObjectsAreTheSame = gentlyEquals(res, prev);
var sb = new StringBuilder();
sb.append("Adding at ").append(at).append(" object:\n ");
dumpObject(sb, res);
@@ -232,10 +239,57 @@ final class PerInputImpl implements Input {
} else {
throw raise(RuntimeException.class, ex);
}
+ } else {
+ if (!REPORTED.contains(res.getClass())) {
+ var inData2 = new PerInputImpl(cache, at);
+ var res2 = p.readWith(inData2);
+ res2 = cache.resolveObject(res2);
+
+ if (!gentlyEquals(res, res2)) {
+ if (res.getClass() == res2.getClass()) {
+ PerUtils.LOG.error(res.getClass() + " doesn't support equality properly!");
+ REPORTED.add(res.getClass());
+ } else {
+ throw new IllegalStateException(
+ "Two different classes " + res.getClass() + " vs. " + res2.getClass());
+ }
+ }
+ }
}
return res;
}
+ private static final Class<?> IR;
+ private static final Method DUPL;
+ static {
+ Class<?> irClass = null;
+ Method dupl = null;
+ try {
+ irClass = Class.forName("org.enso.compiler.core.IR");
+ dupl = irClass.getMethod("duplicate", boolean.class, boolean.class, boolean.class, boolean.class);
+ } catch (ReflectiveOperationException ex) {
+ }
+ IR = irClass;
+ DUPL = dupl;
+ }
+ private static boolean gentlyEquals(Object o1, Object o2) {
+ if (Objects.equals(o1, o2)) {
+ return true;
+ }
+ if (IR != null && IR.isInstance(o1) && IR.isInstance(o2)) {
+ try {
+ var c1 = DUPL.invoke(o1, true, false, false, true);
+ var c2 = DUPL.invoke(o2, true, false, false, true);
+ if (Objects.equals(c1, c2)) {
+ return true;
+ }
+ } catch (IllegalAccessException | InvocationTargetException ex) {
+ ex.printStackTrace();
+ }
+ }
+ return false;
+ }
+
private static void dumpObject(StringBuilder sb, Object obj) {
sb.append(obj.getClass().getName())
.append("@")
the approach however doesn't fully solve the problem either yet. |
The diff is working, but a bit too complex. Let's make it simpler by providing |
2 tasks
github-project-automation
bot
moved this from 👁️ Code review
to 🟢 Accepted
in Issues Board
|
Jaroslav Tulach reports a new STANDUP for yesterday (2024-09-30): Progress: .
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(src https://github.com/enso-org/enso/actions/runs/10716335701/job/29713608555#step:7:1365)
The text was updated successfully, but these errors were encountered: