Fixes to Persistance #10101

radeusgd · 2024-05-27T16:32:47Z

Pull Request Description

Follow-up to Support cycles in the serialized IR structure #9361
- Enables assertions and fixes count check
- Tests and fixes null references
- Tests and fixes serializing a deserialized structure - by saving the id of the Persistance corresponding to the entry
After the change to how we determine which Persistance instance to use, the most specific one is now used (based on the saved id). This has an unfortunate consequence that Seq which is most of the time represented by a subtype of List, is now using PersistScalaList which is not lazy.
To alleviate that, we no longer use Seq to store some field lazily and instead use a dedicated type for that purpose: InlineReference.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

…e Reference" This reverts commit c4d0e72.

…reading the reference table a bit clearer

…ance-followup

JaroslavTulach · 2024-05-28T13:45:08Z

There is an advice describing how to generate a javadoc. I had to add a classpath:

find lib/java/persistance/src/main/java/ | grep java$ | xargs ~/bin/graalvm-21/bin/javadoc -d target/javadoc/ -cp $HOME/.m2/repository/org/slf4j/slf4j-api/2.0.7/slf4j-api-2.0.7.jar:$HOME/.m2/repository/org/netbeans/api/org-openide-util-lookup/RELEASE150/org-openide-util-lookup-RELEASE150.jar --snippet-path lib/java/persistance/src/test/java/

then the Javadoc appeared in target/javadoc/index.html

There is a new class without Javadoc PersistInlineReference - probably unexpected. Then there is the InlineReference with Javadoc - I am assuming, it is expected.

JaroslavTulach

I'll play with the PR more.

JaroslavTulach · 2024-05-28T14:27:38Z

build.sbt

@@ -1258,6 +1258,7 @@ lazy val `persistance` = (project in file("lib/java/persistance"))
    commands += WithDebugCommand.withDebug,
    frgaalJavaCompilerSetting,
    Compile / javacOptions := ((Compile / javacOptions).value),
+    inConfig(Compile)(truffleRunOptionsSettings),


JaroslavTulach · 2024-05-28T14:31:09Z

engine/runtime-parser/src/test/java/org/enso/compiler/core/IrPersistanceTest.java

-    LazySeq.forbidden = true;
-    var out = serde(java.util.Map.class, in, 72);
+    LazyString.forbidden = true;
+    var out = serde(java.util.Map.class, in, 76);


Increasing these numbers is OK due to the consequences of changing the serialization format.

JaroslavTulach · 2024-05-28T14:49:24Z

lib/java/persistance-dsl/src/main/java/org/enso/persist/impl/PersistableProcessor.java

    var tu = processingEnv.getTypeUtils();
    var cnt = 0;
    for (var p : parameters) {
      var type = tu.asElement(tu.erasure(p.asType()));
-      if (type != null && type.getSimpleName().toString().equals("Seq")) {
+      if (type != null && type.getSimpleName().toString().equals("InlineReference")) {


I've almost forgotten about this hack.

JaroslavTulach · 2024-05-28T14:55:25Z

lib/java/persistance/src/main/java/org/enso/persist/PerGenerator.java

+        assert position == NULL_REFERENCE_ID;
+        out.writeInt(NULL_REFERENCE_ID);
+      } else {
+        out.writeInt(position);


I have to think why this is necessary and whether there isn't a better solution. When using Output.writeObject the persistance ID is written.

Only when using writeInline it is not - because it is expected one knows the inlined type. But objects written inline shall not be referenceble from outside.

Strange.

JaroslavTulach · 2024-05-28T14:56:51Z

lib/java/persistance/src/main/java/org/enso/persist/PerGenerator.java


    var refsOut = new ByteArrayOutputStream();
    var refsData = new DataOutputStream(refsOut);
    refsData.writeInt(-1); // space for size of references
-    refsData.writeInt(objAt); // the main object
+    root.writePositionAndPersistanceId(refsData);


Storing the type for all roots is seems however fine so probably my comments aren't that important.

JaroslavTulach · 2024-05-28T15:11:49Z

lib/java/persistance/src/main/java/org/enso/persist/Persistance.java

+   * use {@link Reference} instead. It should be used when we want to ensure that a part of a
+   * structure is only read lazily.
+   */
+  public abstract static sealed class InlineReference<T>


I don't like two classes doing the same, but having different type. Such an approach looks like Text | Column | Regex union type, but that's not how object orientation works. According to On understanding, data abstraction OOP prefers common interface with multiple implementation. Because we are designing an API in object oriented language, I prefer it as well.

While working on #10117 I also noticed that the APIs are slightly different. Reference has get with parameter. InlineReference has get without parameter. I'd say consistency matters and unless there is a reason why the original decision was wrong, I'd stick with it.

PR #10117 solves this inconsistency by exposing only Persistance.Reference abstract class.

JaroslavTulach · 2024-05-28T15:12:14Z

lib/java/persistance/src/main/java/org/enso/persist/Persistance.java

+  }
+
+  @ServiceProvider(service = Persistance.class)
+  public static final class PersistInlineReference extends Persistance<InlineReference> {


This class is accidentally visible in the Javadoc.

In the #10117 PR I made the class package private and registered it manually in PerMap static initializer.

JaroslavTulach · 2024-05-28T17:06:37Z

I've created a PR on top of this PR:

Exposing just a single Persistance.Reference abstraction #10117

I didn't want to commit directly to your PR, @radeusgd as I am not sure all your use-cases are working.

radeusgd · 2024-05-29T09:32:01Z

I've created a PR on top of this PR:

Exposing just a single Persistance.Reference abstraction #10117

I didn't want to commit directly to your PR, @radeusgd as I am not sure all your use-cases are working.

Thank you very much for your propositions. I will rebase my Types PR on top of them and see if the practical use cases are all working - then we can merge the PRs. I will do it when I have my next Types work slot (probably around next week, unless we want to have this finished faster).

* Exposing just a single Persistance.Reference abstraction * Reference.of(obj, deferredWrite) * Encapsulating Reference serde in PerReferencePeristance

…ance-followup

radeusgd added 19 commits May 27, 2024 18:21

add test for storing NULL reference

d08f80a

fix storing NULL references

caf213a

add test for serializing a deserialized loop (it used to be failing)

4a7e358

try fixing by making the writer aware of the inner type of the Reference

b867051

simplify supertype search

86a08a4

Revert "try fixing by making the writer aware of the inner type of th…

db4ec13

…e Reference" This reverts commit c4d0e72.

store Persistance Ids next to references

6600f60

add debug info

d073c3d

enable asserts in persistance tests

63f4e8a

fix sizes after protocol update

4c7e444

rename for clarity

fb3f6ce

introduce InlineReference

8376869

better error message

3d9bd75

move java List persistance to IrPersistance

57d98bf

count InlineReferences instead of Sequences

ca1fd9d

update test

6d68b8a

ensure persistance of List and Seq is aligned

12e7cf3

cleanup after cherry pick

65adea6

update count at the right moment

f50063b

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label May 27, 2024

radeusgd requested review from 4e6, JaroslavTulach, hubertp and Akirathan as code owners May 27, 2024 16:32

radeusgd added 3 commits May 27, 2024 18:46

cleanup

e74d7ea

null

1de43b1

Avoid storing unnecessary integer for NULL references, make the code …

1a2b251

…reading the reference table a bit clearer

radeusgd mentioned this pull request May 27, 2024

Type Inference PoC - iteration 1 - most basic type inference #8652

Merged

5 tasks

enso-bot bot mentioned this pull request May 27, 2024

Prototype for type inference IR pass #8590

Closed

6 tasks

radeusgd added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 27, 2024

Merge branch 'refs/heads/develop' into wip/radeusgd/cycles-in-persist…

f0e9ffa

…ance-followup

JaroslavTulach reviewed May 28, 2024

View reviewed changes

JaroslavTulach mentioned this pull request May 28, 2024

Exposing just a single Persistance.Reference abstraction #10117

Merged

Exposing just a single Persistance.Reference abstraction (#10117)

3f8e144

* Exposing just a single Persistance.Reference abstraction * Reference.of(obj, deferredWrite) * Encapsulating Reference serde in PerReferencePeristance

radeusgd requested a review from JaroslavTulach June 1, 2024 13:09

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 1, 2024

JaroslavTulach approved these changes Jun 2, 2024

View reviewed changes

radeusgd added 3 commits June 8, 2024 13:01

Merge branch 'refs/heads/develop' into wip/radeusgd/cycles-in-persist…

86adcfb

…ance-followup

add missing persistance module to language-server tests

f239249

Merge branch 'refs/heads/develop' into wip/radeusgd/cycles-in-persist…

38b21c4

…ance-followup

mergify bot merged commit 99a1d05 into develop Jun 10, 2024
36 checks passed

mergify bot deleted the wip/radeusgd/cycles-in-persistance-followup branch June 10, 2024 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to Persistance #10101

Fixes to Persistance #10101

radeusgd commented May 27, 2024

JaroslavTulach commented May 28, 2024

JaroslavTulach left a comment

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024 •

edited

Loading

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024 •

edited

Loading

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024

JaroslavTulach May 28, 2024

JaroslavTulach commented May 28, 2024

radeusgd commented May 29, 2024

Fixes to Persistance #10101

Fixes to Persistance #10101

Conversation

radeusgd commented May 27, 2024

Pull Request Description

Important Notes

Checklist

JaroslavTulach commented May 28, 2024

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach May 28, 2024

Choose a reason for hiding this comment

JaroslavTulach commented May 28, 2024

radeusgd commented May 29, 2024

JaroslavTulach May 28, 2024 •

edited

Loading

JaroslavTulach May 28, 2024 •

edited

Loading