Make suspended atom fields work for boxed atoms #8712

JaroslavTulach · 2024-01-09T07:15:21Z

Pull Request Description

Fixes #8710 by making sure suspended atom fields support works also for "normal" Atom instances without any special Layout. Moves all atom related classes into single package and hides as much of classes as possible by making them package private.

Checklist

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

All code follows the
Scala,
Java,
style guides.
All code has been tested:
- Unit tests have been written where possible.
- Check engine benchmarks
- Rewrite newInstance - done in f8d8a98
- Check engine benchmarks, another run, 3rd run
- Polish documentation: done.

JaroslavTulach · 2024-01-09T10:37:31Z

OK, now we have two test failures that need to be fixed.

…omFields_8710

….atom package

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/atom/AtomConstructor.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java

…egies

JaroslavTulach · 2024-01-15T10:48:08Z

After a28daff we only expose four essential classes from the "atom" package:

…f AtomConstructors

JaroslavTulach · 2024-01-15T14:57:52Z

Running engine benchmarks with 08df824 fixes that optimize fields extraction in ConstructorBranchNode.

JaroslavTulach · 2024-01-16T02:39:41Z

...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java

@@ -739,6 +738,10 @@ public static FunctionPointer fromAtomConstructor(AtomConstructor atomConstructo
    }

    public static FunctionPointer fromFunction(Function function) {
+      var cons = AtomConstructor.accessorFor(function);


Introducing the accessorFor method allows us to make QualifiedAccessorNode package private, @4e6.

JaroslavTulach · 2024-01-16T02:43:56Z

...time/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ConstructorBranchNode.java

    if (profile.profile(matcher == target.getConstructor())) {
-      accept(frame, state, structs.getFields(target));
+      var arr = toArrayNode.executeFields(target);


The benchmarks regressed a bit after removal of getFields, but introduction of FieldsAsArrayNode that provides a polymorphic cache for each AtomConstructor (up to limit 3 - usually there is only one anyway) and explodes the loop that reads the fields one by one seems to deliver the same results.

JaroslavTulach · 2024-01-16T02:46:50Z

...src/main/java/org/enso/interpreter/node/expression/builtin/UniquelyConstructibleBuiltin.java

@@ -29,6 +30,6 @@ protected void postInitialize() {
  }

  public final Atom newInstance(Object... params) {
-    return uniqueConstructor.newInstance(params);
+    return AtomNewInstanceNode.getUncached().newInstance(uniqueConstructor, params);


I decided to hide AtomConstructor.newInstance as package private and expose AtomNewInstanceNode.getUncached() instead. It was not clear that AtomConstructor.newInstance was slow (and only allocating BoxedAtom!). Now, when we follow standard Truffle conventions, it is clear getUncached() is the slow path version of AtomNewInstanceNode.create(). Functionally they are identical as AtomConstructorTest demonstrates.

JaroslavTulach · 2024-01-16T02:54:18Z

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/GetAtomFieldsNode.java

-  EnsoObject doStruct(Atom atom, @CachedLibrary(limit = "2") StructsLibrary structs) {
-    return ArrayLikeHelpers.wrapObjectsWithCheckAt(structs.getFields(atom));
+  @TruffleBoundary
+  final EnsoObject doOther(Atom atom) {


Question of Truffle experts: If I do just the first @Specialization(limit = LIMIT, guards = {"atom.getConstructor() == cons"}) then the execute method yields UnsupportedSpecializationException when we send there more than LIMIT of different atom constructors.

I thought @Fallback would be appropriate, but DSL processor refuses it. So I ended up with @Specialization without any guards behind @TruffleBoundary calling the first specialization @TruffleBoundary. Can we do better? Should the second specialization replace the first one?

JaroslavTulach · 2024-01-16T02:57:54Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Type.java

-                    new GetFieldNode(language, field.getPosition(), this, field.getName()));
-              });
-    }
+    var roots = AtomConstructor.collectFieldAccessors(language, this);


Introduction of collectFieldAccessors function allows us to hide GetFieldWithMatchNode and GetFieldNode and make them package private.

JaroslavTulach · 2024-01-16T03:01:31Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/atom/AtomConstructor.java

-    if (Layout.isAritySupported(args.length)) {
-      boxedLayout = Layout.create(args.length, 0, args);
-    }
+    boxedLayout = Layout.createBoxed(args);


The major change that fixes #8710:

each AtomConstructor is associated with boxedLayout

we always use StructsLibrary (and thus layouts) to access the atom values

as such the SuspendedFieldGetterNode is used for BoxingAtom as well as UnboxingAtom

JaroslavTulach · 2024-01-16T03:16:12Z

...e/runtime/src/main/java/org/enso/interpreter/runtime/data/atom/SuspendedFieldGetterNode.java

@@ -33,7 +36,51 @@ private SuspendedFieldGetterNode(
    this.set = set;
  }

-  static UnboxingAtom.FieldGetterNode build(
+  static NodeFactory<? extends UnboxingAtom.FieldGetterNode> factory(


Wrapping of FieldGetterNode now happens on the level of factories. As such it should be more consistent and the suspended wrapper shall be used everywhere.

JaroslavTulach · 2024-01-16T03:24:10Z

engine/runtime/src/test/java/org/enso/interpreter/test/AtomConstructorTest.java

+    assertAtomFactory("AtomNewInstanceNode.create", factory);
+    assertLessArguments("AtomNewInstanceNode.create", factory);
+
+    assertAtomFactory("AtomConstructor.newInstance with priming", factory);


There are two tests. First one verifies the behavior of getUncached() in isolation. The second one checks whether both cached and uncached variants co-operate well.

JaroslavTulach · 2024-01-16T03:25:07Z

engine/runtime/src/test/java/org/enso/interpreter/test/LazyAtomFieldTest.java

+  }
+
+  @Test
+  public void fiveAtomIntFields() throws Exception {


This is the test that simulates #8710 - fourXYZ tests were always passing. fiveXYZ tests were failing prior to this PR.

…omFields_8710

hubertp

Minor questions about the (need for) TruffleBoundary annotations

hubertp · 2024-01-16T11:50:49Z

...e/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyPrettyNode.java

-  private Text doComplexAtom(Atom atom, Object[] fields) {
-    Text res = Text.create("(", consName(atom.getConstructor()));
+  private Text doComplexAtom(Atom atom) {
+    var structs = StructsLibrary.getUncached();


Why not keep on using a cached StructsLibrary in doAtom and passing it here?

Will it make anything better if doComplexAtom is inlined into PE mode? Do we really need to run this code at assembly speed? Please note that there are showObject calls that are annotated with @TruffleBoundary anyway.

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/AtomWithAHoleNode.java

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/GetAtomFieldsNode.java

...e/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToTextNode.java

...untime/src/main/java/org/enso/interpreter/runtime/data/atom/AtomConstructorInstanceNode.java

JaroslavTulach added 4 commits January 9, 2024 06:49

Four tests showing difference between four and five atom fields

4aa19cc

Separate evalCode and execute

c227a5b

Avoid public BoxingAtom constructor

9bc1af4

Keeping atoms in the same package allows packge private communication

597599a

JaroslavTulach added --bug Type: bug -compiler labels Jan 9, 2024

JaroslavTulach requested a review from jdunkerley January 9, 2024 07:15

JaroslavTulach self-assigned this Jan 9, 2024

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners January 9, 2024 07:15

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 9, 2024

JaroslavTulach marked this pull request as draft January 9, 2024 10:38

enso-bot bot mentioned this pull request Jan 10, 2024

Avoid Enso/Python & co. interop problems by using Truffle TCK tests #5238

Closed

JaroslavTulach added 5 commits January 12, 2024 04:35

Merge remote-tracking branch 'origin/develop' into wip/jtulach/RichAt…

22f22ae

…omFields_8710

Using Layout to access BoxingAtom

b543c7c

Explicitly force creation of BoxedLayout

bb6116b

Moving everything Atom related into org.enso.interpreter.runtime.data…

4b3e245

….atom package

Always use StructsLibrary when accessing fields

7146ee8

JaroslavTulach commented Jan 13, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/atom/AtomConstructor.java Outdated Show resolved Hide resolved

JaroslavTulach added 2 commits January 13, 2024 05:16

Put creation of a node behind @TruffleBoundary for now

6d890de

Merging with latest develop and resolving AssertNode conflict

7ff995e

JaroslavTulach force-pushed the wip/jtulach/RichAtomFields_8710 branch from e2dbd65 to 7ff995e Compare January 13, 2024 04:24

Turn getMaxUnboxingLayouts into static constant

580c3d8

JaroslavTulach force-pushed the wip/jtulach/RichAtomFields_8710 branch from 32d36d7 to 580c3d8 Compare January 13, 2024 04:52

JaroslavTulach commented Jan 13, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java Show resolved Hide resolved

JaroslavTulach added 3 commits January 13, 2024 06:06

Moving the instantiation logic into own class

03ed29e

Refactoring AtomNewInstanceNode and co.

85c396f

AtomConstructorTest to check consistency of various allocations strat…

d433218

…egies

JaroslavTulach added 5 commits January 15, 2024 12:39

Use AtomNewInstanceNode.newInstance to instantiate AtomConstructor

b6a1c2a

Pass cons into newInstance as first parameter

0c5955a

Support endless specializations

fff48c5

AtomConstructorInstanceNode must be able to handle unlimited number o…

2fe3a87

…f AtomConstructors

FieldsAsArrayNode to speed conversions up

08df824

Fixing javadoc typo

6bb2725

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 16, 2024

enso-bot bot mentioned this pull request Jan 16, 2024

Rich atoms don't cache suspended fields #8710

Closed

JaroslavTulach commented Jan 16, 2024

View reviewed changes

JaroslavTulach added 2 commits January 16, 2024 04:04

It is @see tag

33e79e1

CompilerDirectives.shouldNotReachHere

a5b169c

JaroslavTulach commented Jan 16, 2024

View reviewed changes

Test getUncached() with priming

3addd39

JaroslavTulach commented Jan 16, 2024

View reviewed changes

Akirathan approved these changes Jan 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into wip/jtulach/RichAt…

2eee9fa

…omFields_8710

hubertp approved these changes Jan 16, 2024

View reviewed changes

Removing leftover comment

d62e6b9

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jan 16, 2024

mergify bot merged commit 09f484f into develop Jan 16, 2024
26 of 28 checks passed

mergify bot deleted the wip/jtulach/RichAtomFields_8710 branch January 16, 2024 20:20

enso-bot bot mentioned this pull request Jan 17, 2024

Race in application of pending edits leads to stability issues #8770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make suspended atom fields work for boxed atoms #8712

Make suspended atom fields work for boxed atoms #8712

JaroslavTulach commented Jan 9, 2024 •

edited

JaroslavTulach commented Jan 9, 2024

JaroslavTulach commented Jan 15, 2024

JaroslavTulach commented Jan 15, 2024

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024 •

edited

JaroslavTulach Jan 16, 2024 •

edited

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024

JaroslavTulach Jan 16, 2024

hubertp left a comment

hubertp Jan 16, 2024

JaroslavTulach Jan 16, 2024

Make suspended atom fields work for boxed atoms #8712

Make suspended atom fields work for boxed atoms #8712

Conversation

JaroslavTulach commented Jan 9, 2024 • edited

Pull Request Description

Checklist

JaroslavTulach commented Jan 9, 2024

JaroslavTulach commented Jan 15, 2024

JaroslavTulach commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Jan 16, 2024 • edited

Choose a reason for hiding this comment

JaroslavTulach Jan 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Jan 9, 2024 •

edited

JaroslavTulach Jan 16, 2024 •

edited

JaroslavTulach Jan 16, 2024 •

edited