Simplify lazy type checking of listings/mappings #789

odenix · 2024-11-07T20:16:32Z

I marked this PR as a draft because my initial goal is to start a conversation. For details see commit messages.

bioball

I haven't dug deep yet, but I'm surprised this passes! Using the amends chain affects the semantics of super. This feels somewhat risky, and perhaps there is a semantic that isn't yet covered by our language snippet tests.

I also don't think this is great for perf. Using the amends chain means that every child receives its own complete set of cached members every time, and also means computing the same value multiple times (which you've seen by the now deleted listing7.pkl).
For large listings/mappings, I think this will get quite expensive.

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

bioball · 2024-11-11T21:49:59Z

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

-        }
-        ret = vmListingOrMapping.typecastObjectMember(member, ret, callNode);
+        // `if (receiver instanceof VmListingOrMapping)` doesn't work
+        // (only) because PropertiesRenderer amends a VmDynamic with a VmListing (hack?)


Umm... probably a bug

To me this looks like a quick and efficient hack. But I think it would be better not to break the invariant that the entire amends chain below the prototype (which is VmTyped) uses the same subclass of VmObject.

Yeah, I think you're right. And yeah, should remove that hack.

The hack wasn't as easy to remove as I had thought, so I'll leave this for another PR.

bioball · 2024-11-11T21:54:42Z

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java

+  @Specialization
+  protected Object evalMapping(VirtualFrame frame, VmMapping receiver) {
+    var result = executeBody(frame);
+    return receiver.doTypeCast(result, VmUtils.getOwner(frame), callNode, null, null);


Haven't verified anything yet, but, It doesn't seem correct to call doTypeCast both here and in VmUtils.

VmUtils.doReadMember only typecasts constant members, whereas ElementEntryNode is only used for non-constant members. This now works the same for elements, entries, and properties.

Gotcha! Okay, that makes sense.

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java

odenix · 2024-11-11T23:54:11Z

Using the amends chain affects the semantics of super.

Does it? Note that a typecast results in an object with no members.
(In many cases, the extra object could be optimized away, but that's a separate concern.)

I also don't think this is great for perf. Using the amends chain means that every child receives its own complete set of cached members every time, and also means computing the same value multiple times (which you've seen by the now deleted listing7.pkl).

I think this PR could implement the same optimization that exists in the current code.
However, it's not clear to me that this optimization is beneficial in the real world:

it requires populating and querying two extra maps cachedMembers and checkedMembers
it breaks the following invariant, which can often be exploited to improve performance:
- an object whose members have all been evaluated has a fully populated cachedValues map
in many cases, the typecast is done implicitly by the interpreter, and the original object is never shared or evaluated. Example:
```
// foo has type `Listing<String>`
foo { "element" }
```
In the above code, foo { "element" } is never shared as accessing foo returns foo { "element" } as Listing<String>.
Hence caching values for foo { "element" } instead of foo { "element" } as Listing<String> has no benefit (only a cost).

I think that my proposed optimization "avoid creating an intermediate untyped listing/mapping", which isn't possible with the current data structure, is more likely to result in real-world gains.
Unfortunately, I don't have access to large real-world Pkl programs to test my performance hypotheses.
I've considered generating large programs, but such programs may not be indicative of real-world performance.

bioball · 2024-11-12T23:35:33Z

If we don't share cached members, every node on m1 gets executed twice; assuming that both m1 and m2 are in the path of evaluation. It also means that we double the amount of cached members, and double the allocations (more VmObjects, VmValue, etc).

m1: Mapping = new {
  ["foo"] {
    bar {
      baz = doSomething()
    }
  }
}

m2: Mapping<String, Dynamic> = m1

Here's a "real-world" ish piece of code that ends up being much more costly (run with pkl eval -m <output_dir> and see two traces):

import "package://pkg.pkl-lang.org/pkl-k8s/[email protected]#/api/core/v1/Pod.pkl"
import "package://pkg.pkl-lang.org/pkl-k8s/[email protected]#/K8sResource.pkl"

local pods: Listing<Pod> = new {
  new {
    metadata {
      name = trace("my-cool-pod")
      namespace = "dev"
    }
  }
}

local allPodNames = gatherNames(pods)

function gatherNames(resources: Listing<K8sResource>) =
  resources.toList().map((k) -> k.metadata.name)

output {
  files {
    for (p in pods) {
      ["\(p.metadata.name).yaml"] = p.output
    }
    ["all-pods-names.yaml"] {
      value = allPodNames
      renderer = new YamlRenderer {}
    }
  }
}

I think the current approach probably makes the best trade-off here; we have extra fields for every VmMapping and VmListing, but computing every node twice is much more expensive, and, on average, it's a big perf gain.

bioball · 2024-11-12T23:43:42Z

BTW, quick note: your proposed changes introduces this regression; so we definitely have some corner cases not caught by our language snippet tests:

Given:

class Person { name: String }

people1: Listing<Person> = new {
  new {
    name = "Sandy"
  }
}

people2: Listing<Person(name.endsWith("Smith"))> = (people1) {
  [[true]] {
    name = super.name + " Smith"
  }
}

The changes in this PR produces:

–– Pkl Error ––
Type constraint `name.endsWith("Smith")` violated.
Value: new Person { name = "Sandy" }

9 | people2: Listing<Person(name.endsWith("Smith"))> = (people1) {
                            ^^^^^^^^^^^^^^^^^^^^^^
at test#people2 (/Users/danielchao/code/apple/pkl/.dan-scripts/test.pkl:9)

4 | new {
    ^^^^^
at test#people1[#1] (/Users/danielchao/code/apple/pkl/.dan-scripts/test.pkl:4)

106 | text = renderer.renderDocument(value)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (https://github.com/apple/pkl/blob/6a793b7c5/stdlib/base.pkl#L106)

odenix · 2024-11-14T18:18:36Z

BTW, quick note: your proposed changes introduces this regression; so we definitely have some corner cases not caught by our language snippet tests:

Fixed here: 724482d

odenix · 2024-11-14T23:23:32Z

I think the current approach probably makes the best trade-off here; we have extra fields for every VmMapping and VmListing, but computing every node twice is much more expensive, and, on average, it's a big perf gain.

Your first example is a downcast caused by partial typing. Intuitively, it feels OK to me that a downcast, which shouldn't be the norm, incurs some overhead. Note that if m2 amends m1, the current optimization won't help.

Your second example is an upcast. This case feels more relevant to me because it cannot be easily avoided. Here, a better optimization is to recognize (and cache) the fact that no type cast is required because Listing<Pod> is a subtype of Listing<K8sResource>.

The number of root node calls made in a limited set of circumstances (*) shouldn't be the only metric an implementation is judged by. I see the following problems with the current implementation, some of which are easier to fix than others:

adds a lot of complexity
adds a lot of state (5 fields and 2 maps per VmListing instance)
makes many VmObjectLike/VmListingOrMapping virtual method calls, which are best avoided in Truffle interpreters
makes accessing cached values more expensive
makes iterating cached values more expensive
Can't avoid an intermediate untyped VmListing in the common case that a property of type Listing<X> is amended in place.
That's because the current VmListing can't have both a type cast node and new members.
typeNodeFrame seems unnecessary
EDIT: It may be necessary in the current implementation, but isn't in my implementation.
checkedMembers seems unnecessary
I think cachedValues could be used instead.
skips required type casts
Executing type casts in receiver's and owner's delegate chain isn't sufficient.
uses Objects.requireNonNull as assertion in interpreter code
This is a small but unnecessary overhead. requireNonNull is primarily intended for validating method arguments in public APIs and throws NullPointerException.

(*) direct assignment without amendment, type cast cannot be elided, many computed members, computations are expensive

bioball · 2024-11-17T23:08:08Z

Your first example is a downcast caused by partial typing. Intuitively, it feels OK to me that a downcast, which shouldn't be the norm, incurs some overhead.

I think it might be fine if it was a little bit of overhead, but, in this case, it's twice the overhead (in both space and time). That's quite a lot, and can make it hard for users to reason through the performance of a program. This means that changing or adding a type annotation, which seems quite innocent, might double the cost of something. In large codebases, this can become quite problematic.

Responding to the rest of the concerns:

adds a lot of complexity

adds a lot of state (5 fields and 2 maps per VmListing instance)

Indeed--it would be good to reduce complexity, but I think we should be sharing cached members whenever possible.

makes many VmObjectLike/VmListingOrMapping virtual method calls, which are best avoided in Truffle interpreters

Sorry; can you elaborate on "virtual method call"? Do you mean dynamic dispatch?

makes accessing cached values more expensive

makes iterating cached values more expensive

That's true, although, In the common case, I think this is pretty marginal. In the following code, all of the cached values are stored directly on the delegated (new Listing { 1; 2; 3 } as Listing<Int>), rather than on the delegate (new Listing { 1; 2; 3 }).

foo = new Listing { 1; 2; 3 } as Listing<Int>

As long as cached values are stored on the delegated, member lookups are just as cheap as they used to be.

Can't avoid an intermediate untyped VmListing in the common case that a property of type Listing<X> is amended in place.
That's because the current VmListing can't have both a type cast node and new members.

We can probably optimize the case of new Listing<Int> { 1; 2; 3 } even in the current implementation

typeNodeFrame seems unnecessary
EDIT: It may be necessary in the current implementation, but isn't in my implementation.

Might be unnecessary in the current implementation too; need to investigate this (thanks for pointing this out!)

checkedMembers seems unnecessary
I think cachedValues could be used instead.

It's indeed not necessarily; it's there as an optimization, although, it might not be saving that much.

skips required type casts
Executing type casts in receiver's and owner's delegate chain isn't sufficient.

Are you referring to #785? If so, this is fixed in #822.

uses Objects.requireNonNull as assertion in interpreter code
This is a small but unnecessary overhead. requireNonNull is primarily intended for validating method arguments in public APIs and throws NullPointerException.

Good point! We should switch to assert for these.

odenix · 2024-11-18T01:45:04Z

I think it might be fine if it was a little bit of overhead, but, in this case, it's twice the overhead (in both space and time). That's quite a lot, and can make it hard for users to reason through the performance of a program. This means that changing or adding a type annotation, which seems quite innocent, might double the cost of something. In large codebases, this can become quite problematic.

I think you may be focusing too much on this particular optimization instead of the bigger optimization picture. Anyway, I brought back the optimization and corresponding test here: 2ec4124

Another flaw of this optimization, or at least its current implementation, is that it only works if the member is, coincidentally, first evaluated for the parent/delegate. This can be verified by changing the order of listing1 and listing2 in listing7.pkl.

That's true, although, In the common case, I think this is pretty marginal. In the following code, all of the cached values are stored directly on the delegated (new Listing { 1; 2; 3 } as Listing), rather than on the delegate (new Listing { 1; 2; 3 }).

I think checkedMembers prevents values from being cached in a single place. Hence cached values can no longer be iterated by iterating an (economic) map, which is much faster than doing a map lookup for each value.

Sorry; can you elaborate on "virtual method call"? Do you mean dynamic dispatch?

Yes. Several VmObject methods are no longer final.

We can probably optimize the case of new Listing { 1; 2; 3 } even in the current implementation

Yes, but not the amendment case.

It's indeed not necessarily; it's there as an optimization, although, it might not be saving that much.

What makes checkedMembers a (small) improvement over cachedValues?

bioball · 2024-11-20T23:52:47Z

I think you may be focusing too much on this particular optimization instead of the bigger optimization picture. Anyway, I brought back the optimization and corresponding test here: 2ec4124

Another flaw of this optimization, or at least its current implementation, is that it only works if the member is, coincidentally, first evaluated for the parent/delegate. This can be verified by changing the order of listing1 and listing2 in listing7.pkl.

We're certainly on two sides here. You are right that this optimization is only relevant for certain cases of Pkl code. But running into it would otherwise be a significant perf penalty. Doing extra evaluation is expensive, and should be avoided if possible. But thanks for adding this optimization back! I will take a look.

What makes checkedMembers a (small) improvement over cachedValues?

Going through the existing code again, I realized that I introduced checkedMembers at first because the delegatee was not receiving its own cached members. It's actually a result of iterating on the implementation and not re-visiting old assumptions.

bioball

Overall, these changes look good to me! This is an improvement over how the code is written today.

I have some comments, but they're all relatively minor.

Is this PR intended to still be a draft? What else is needed here?

bioball · 2024-11-22T02:36:03Z

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java

+  @Specialization
+  protected Object evalMapping(VirtualFrame frame, VmMapping receiver) {
+    var result = executeBody(frame);
+    return receiver.doTypeCast(result, VmUtils.getOwner(frame), callNode, null, null);


Gotcha! Okay, that makes sense.

bioball · 2024-11-22T02:41:28Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

  private final @Nullable ListingOrMappingTypeCastNode typeCastNode;
-  private final MaterializedFrame typeNodeFrame;


I think we still need this, to address #823. This is an existing regression, that your PR doesn't solve either.

However, this comment isn't blocking because your PR isn't introducing a new regression.

fixed in bc10eba

bioball · 2024-11-22T02:47:41Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

+   * of {@code typeNode}. (If {@code true}, it is redundant to check that elements/values have type
+   * {@code typeNode}.)
+   */
+  public final boolean valueTypeIsSubtypeOf(TypeNode typeNode) {


[nit] "subtype" is kind of a misnomer; there's lots of subtype relationships that this would return false for (e.g. "foo" is a subtype of "foo"|"bar").

I'd prefer if this kept the original name (hasSameCheckAs). Would also be open to other suggestions.

The method name is shorthand for "value type is known to be subtype of" (see doc comment, happy to rename). The current implementation isn't much more than "has same checks as", which is a valid implementation of "is known to be subtype of". However, the implementation should be improved to handle as many subtype relationships as feasible and cache them, which should improve performance considerably.

I also like hasSameCheckAs more, or something of the sort. It's more of an equivalence check. It doesn't really care about subtyping relationships.

I see the value of aspirational naming, but then, ideally, we'd have a TODO comment and a linked Issue. If it stays aspirational beyond the point of us remembering, it becomes misleading.

bioball · 2024-11-22T02:49:22Z

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

@@ -261,17 +262,17 @@ public static Object doReadMember(

    final var constantValue = member.getConstantValue();
    if (constantValue != null) {
-      var ret = constantValue;
-      // for a property, do a type check
+      Object result = constantValue;


Suggested change

Object result = constantValue;

var result = constantValue;

Doesn't seem to be fixed.

bioball · 2024-11-22T02:53:53Z

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

-        }
-        ret = vmListingOrMapping.typecastObjectMember(member, ret, callNode);
+        // `if (receiver instanceof VmListingOrMapping)` doesn't work
+        // (only) because PropertiesRenderer amends a VmDynamic with a VmListing (hack?)


Yeah, I think you're right. And yeah, should remove that hack.

bioball · 2024-11-22T05:31:09Z

pkl-core/src/main/java/org/pkl/core/runtime/VmMapping.java

+
+    return properties;
+  }
+


Accidentally added this? This method isn't used anywhere.

Removed. This was accidentally added by reverting your commit 753dd38.

It's still there.

bioball · 2024-11-22T05:34:43Z

pkl-core/src/test/files/LanguageSnippetTests/input/listings/listingBug785.pkl

+local c = (b) { new Listing { 1 } }
+local d = c as Listing<Listing<Int>>
+
+result = d


Can we rename this to inputs/errors/listingTypeCheckError8.pkl?

Still has the old name.

bioball · 2024-11-22T06:38:05Z

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java

+  protected Object evalListing(VirtualFrame frame, VmListing receiver) {
+    var result = executeBody(frame);
+    return VmUtils.shouldRunTypeCheck(frame)
+        ? receiver.doTypeCast(result, VmUtils.getOwner(frame), callNode, null, null)
+        : result;
+  }
+
+  @Specialization
+  protected Object evalMapping(VirtualFrame frame, VmMapping receiver) {


Let's avoid creating IndirectCallNode for evalDynamic, and also defer until they are needed. We can use the @Cached helper for this.

Suggested change

protected Object evalListing(VirtualFrame frame, VmListing receiver) {

var result = executeBody(frame);

return VmUtils.shouldRunTypeCheck(frame)

? receiver.doTypeCast(result, VmUtils.getOwner(frame), callNode, null, null)

: result;

}

@Specialization

protected Object evalMapping(VirtualFrame frame, VmMapping receiver) {

protected Object evalListing(

VirtualFrame frame, VmListing receiver, @Cached("create()") @Shared("callNode") IndirectCallNode callNode) {

var result = executeBody(frame);

return VmUtils.shouldRunTypeCheck(frame)

? receiver.doTypeCast(result, VmUtils.getOwner(frame), callNode, null, null)

: result;

}

@Specialization

protected Object evalMapping(

VirtualFrame frame, VmMapping receiver, @Cached("create()") @Shared("callNode") IndirectCallNode callNode) {

Another thought: there is another optimization that we can make to these specializations; see

pkl/pkl-core/src/main/java/org/pkl/core/ast/expression/member/ReadPropertyNode.java

Lines 64 to 68 in ad06a96

// This method effectively covers `VmObject receiver` but is implemented in a more

// efficient way. See:

// https://www.graalvm.org/22.0/graalvm-as-a-platform/language-implementation-framework/TruffleLibraries/#strategy-2-java-interfaces

@Specialization(guards = "receiver.getClass() == cachedClass", limit = "99")

protected Object evalObject(

We should be able to mostly copy the code here, and specialize on the receiver being a VmListingOrMapping.

Let's avoid creating IndirectCallNode for evalDynamic

fixed

there is another optimization that we can make to these specializations

I think the optimization in ReadPropertyNode is very specific to the needs of that class. ElementOrEntryNode just needs two specialization methods that specialize on final types VmListing and VmMapping, so spelling them out is much simpler and at least as efficient to execute.

By the way, is it intentational that ReadProperty.checkConst() runs once per node, not once per receiver.getVmClass()?

By the way, is it intentational that ReadProperty.checkConst() runs once per node, not once per receiver.getVmClass()?

Sorry, missed this when I was catching up on messages while I was out.

I think checking once per node is correct. needsConst is only set to true in the case of an implicit this receiver (see the constructor calls in ResolveVariableNode).

And, in the case of the implicit receiver, we only need to check const in the case of class properties.

open class A { a = 1 } class B extends A { const b = a // <-- implicit this lookup of `a`, which is not in a const scope. }

In this case, getVmClass() will always give you the same class.

receiver.getVmClass() could, in general, return a subclass. But I see that Pkl enforces that a property's const-ness doesn't change when subclassing, so this should nevertheless be safe (but also worth a comment).

Can do: #859

bioball · 2024-11-22T06:38:40Z

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java

+  }
+
+  @Specialization
+  protected Object evalDynamic(VirtualFrame frame, @SuppressWarnings("unused") VmDynamic receiver) {


Suggested change

protected Object evalDynamic(VirtualFrame frame, @SuppressWarnings("unused") VmDynamic receiver) {

protected Object evalDynamic(VirtualFrame frame, VmDynamic ignored) {

bioball · 2024-11-22T06:45:17Z

pkl-core/src/main/java/org/pkl/core/util/EconomicMaps.java

+  @TruffleBoundary
+  public static <K, V> UnmodifiableEconomicMap<K, V> emptyMap() {
+    return EconomicMap.emptyMap();
+  }


We don't need a truffle boundary on EconomicMap.emptyMap(). It just casts a singleton, and Truffle should have no trouble doing partial evaluation here.

Removed. We also don't need EconomicMaps. :-)

It's not removed.

The only reason for EconomicMaps was to put truffle boundaries in place, so yeah, no need.

bioball · 2024-11-23T04:56:46Z

By the way, heads up: I'm going to be on vacay for the next two weeks, so you won't hear from me for a bit!

odenix · 2024-11-28T19:34:17Z

You are right that this optimization is only relevant for certain cases of Pkl code

Additionally, whether it works at all depends on evaluation order, which users can't reason about. I'd love to see some memory/cpu benchmarks that prove or disprove its worth. Fortunately, the optimization turned out to be easy to integrate into my implementation of VmListingOrMapping, not requiring changes elsewhere except for making one VmObject method non-final.

By the way, heads up: I'm going to be on vacay for the next two weeks, so you won't hear from me for a bit!

I guess this means further delays for my pending PRs. Anyway, I hope that you are enjoying your vacation!

Is this PR intended to still be a draft? What else is needed here?

Removed draft status.

stackoverflow · 2024-11-29T12:51:59Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

-                getRootNode().getFrameDescriptor(),
-                valueTypeNode,
-                getRootNode().getName());
+                language, new FrameDescriptor(), valueTypeNode, getRootNode().getName());


Why the change to new FrameDescriptor() instead of getting it from the root node?

The correct frame descriptor here is an empty frame descriptor. I don't see a logical connection between this frame descriptor and getRootNode()'s frame descriptor.

stackoverflow · 2024-11-29T13:37:29Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

+   * of {@code typeNode}. (If {@code true}, it is redundant to check that elements/values have type
+   * {@code typeNode}.)
+   */
+  public final boolean valueTypeIsSubtypeOf(TypeNode typeNode) {


I also like hasSameCheckAs more, or something of the sort. It's more of an equivalence check. It doesn't really care about subtyping relationships.

stackoverflow · 2024-11-29T13:43:14Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListing.java

-      var value = getCachedValue(i);
+    var cursor = cachedValues.getEntries();
+    while (cursor.advance()) {
+      Object key = cursor.getKey();


Doesn't seem to be fixed.

stackoverflow · 2024-11-29T13:44:02Z

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

@@ -261,17 +262,17 @@ public static Object doReadMember(

    final var constantValue = member.getConstantValue();
    if (constantValue != null) {
-      var ret = constantValue;
-      // for a property, do a type check
+      Object result = constantValue;


Doesn't seem to be fixed.

stackoverflow · 2024-11-29T13:52:34Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

-      var sourceSection = member.getBodySection();
-      if (!sourceSection.isAvailable()) {
-        sourceSection = member.getSourceSection();
+    if (typeCastNode != null) {


It's not fixed in the last commit (bc10eba).

stackoverflow · 2024-11-29T14:08:25Z

pkl-core/src/main/java/org/pkl/core/runtime/VmMapping.java

+
+    return properties;
+  }
+


It's still there.

stackoverflow · 2024-11-29T14:14:20Z

pkl-core/src/main/java/org/pkl/core/util/EconomicMaps.java

+  @TruffleBoundary
+  public static <K, V> UnmodifiableEconomicMap<K, V> emptyMap() {
+    return EconomicMap.emptyMap();
+  }


It's not removed.

stackoverflow · 2024-11-29T14:15:09Z

pkl-core/src/test/files/LanguageSnippetTests/input/listings/listingBug785.pkl

+local c = (b) { new Listing { 1 } }
+local d = c as Listing<Listing<Int>>
+
+result = d


Still has the old name.

stackoverflow · 2024-11-29T14:18:01Z

Looks good! Only some things that are still not addressed. Maybe you forgot to commit those changes?

odenix · 2024-11-29T16:59:49Z

Maybe you forgot to commit those changes?

Sorry for that, forgot to push. Fixed.

PS: Once the review is complete, I'd like to squash the commits and edit the commit message.

odenix · 2024-11-29T17:14:15Z

I also like hasSameCheckAs more, or something of the sort. It's more of an equivalence check. It doesn't really care about subtyping relationships.

My point is that this should care about subtyping relationships. "Is subtype of" is the desirable check here because listings/mappings are covariant in their element/value type. For example, assigning Listing<Cat> to Listing<Animal> requires the check "Cat is subtype of Animal", but doesn't require additional lazy element checks.

As long as this subtype check is only used to improve performance, it's acceptable to return false instead of true, for example because a particular check is not yet implemented, proves too difficult to implement, or has too high a runtime cost (caching is our friend here). That's why I wrote "is known to be subtype of" in the comment.

stackoverflow

Looks good from my side. Will just wait for @holzensp to do a pass.

holzensp

A few comments / questions, but none of them blocking.

holzensp · 2024-12-02T12:08:37Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

+   * of {@code typeNode}. (If {@code true}, it is redundant to check that elements/values have type
+   * {@code typeNode}.)
+   */
+  public final boolean valueTypeIsSubtypeOf(TypeNode typeNode) {


I see the value of aspirational naming, but then, ideally, we'd have a TODO comment and a linked Issue. If it stays aspirational beyond the point of us remembering, it becomes misleading.

holzensp · 2024-12-02T12:15:37Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

+    // Note that mutating a frame's receiver and owner argument is very risky
+    // because any VmObject instantiated within the same root node execution
+    // holds a reference to (not immutable snapshot of) the frame
+    // via VmObjectLike.enclosingFrame.
+    // *Maybe* this works out for TypeAliasTypeNode because an object instantiated
+    // within a type constraint doesn't escape the constraint expression.
+    // If mutating receiver and owner can't be avoided, it would be safer
+    // to have VmObjectLike store them directly instead of storing enclosingFrame.


Given this danger; why not make a synthetic extra frame, on top of the given frame, with the new owner/receiver?

I see the value of aspirational naming, but then, ideally, we'd have a TODO comment and a linked Issue. If it stays aspirational beyond the point of us remembering, it becomes misleading.

I've now changed the method name to isValueTypeKnownSubtypeOf, at which point it's no longer aspirational.
The method already handles ValueType <: Any and ValueType <: Unknown.
Improving this further is part of (6) on my planned list of improvements.

Given this danger; why not make a synthetic extra frame, on top of the given frame, with the new owner/receiver?

Inserting a frame means inserting a root node call, which I think means abandoning the current implementation strategy of inlining type aliases into the root node referencing them.

Given this danger; why not make a synthetic extra frame, on top of the given frame, with the new owner/receiver?

Turns out this may be possible:

var newArgs = frame.getArguments().clone(); newArgs[0] = newReceiver; newArgs[1] = newOwner; var newFrame = Truffle.getRuntime().createVirtualFrame(newArgs, frame.getDescriptor()); childNode.execute(newFrame);

holzensp · 2024-12-02T12:17:29Z

pkl-core/src/main/java/org/pkl/core/util/EconomicMaps.java

+  @TruffleBoundary
+  public static <K, V> UnmodifiableEconomicMap<K, V> emptyMap() {
+    return EconomicMap.emptyMap();
+  }


The only reason for EconomicMaps was to put truffle boundaries in place, so yeah, no need.

holzensp · 2024-12-02T12:41:30Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListing.java

+    var cursor = cachedValues.getEntries();
+    while (cursor.advance()) {
+      var key = cursor.getKey();
+      if (key instanceof Identifier) continue;


Doesn't this just create more work? I guess this trades lookup cost (of the previous by-index formulation) against instanceof + branching cost.

I'd expect this branch to be rarely taken for a listing. The instanceof check is roughly key.getClass() == Identifier.class), which should be faster than a map lookup. (I didn't write this code, just reverted the commit that made major changes here and elsewhere.)

holzensp · 2024-12-02T12:58:34Z

pkl-core/src/main/java/org/pkl/core/runtime/VmMapping.java

    cachedHash = result;
    return result;
  }

+  // assumes mapping has been forced


Asserting is better than assuming; not against making forced protected.

I didn't write this code, just reverted a commit. This method requires shallow forcing, which isn't currently tracked and hence can't be asserted. (I've added tracking of shallow forcing in one of my planned PRs.)

holzensp · 2024-12-02T13:04:36Z

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

+      } else if (owner instanceof VmListingOrMapping) {
+        result =
+            ((VmListingOrMapping) receiver)
+                .doTypeCast(constantValue, owner, callNode, member, null);


Agreed with "better to remove the hack" above, but this reads weird, even with the comment, maybe...

Suggested change

} else if (owner instanceof VmListingOrMapping) {

result =

((VmListingOrMapping) receiver)

.doTypeCast(constantValue, owner, callNode, member, null);

} else if (owner instanceof VmListingOrMapping && receiver instance VmListingOrMapping checkable) {

result = checkable.doTypeCast(constantValue, owner, callNode, member, null);

changed to (almost) your code

Approved by other maintainers

odenix · 2024-12-02T18:18:07Z

Replied to all feedback. Let me know when it's time to squash and edit the commit message. It would also be great if you could test this PR internally.

stackoverflow · 2024-12-03T10:18:06Z

There seem to be still some rough edges:
foo.pkl

typealias Alias = String(endsWith("x"))

local function isValid(alias: Alias): Boolean = alias.startsWith("a")

foo: Listing<Alias(isValid(this))>(isDistinct)

bar.pkl

amends "foo.pkl"

foo {
  "abcdx"
  "ax"
}

Results in

–– Pkl Error ––
Cannot find method `isValid`.

6 | foo: Listing<Alias(isValid(this))>(isDistinct)

If you remove the isDistinct constraint, or change it to !isEmpty, for example, the file evaluates correctly. Probably because isDistinct has to force all the members.

odenix · 2024-12-03T19:11:41Z

There seem to be still some rough edges

Fixed in 9e7b241. Anything else?

stackoverflow · 2024-12-03T20:37:20Z

There seem to be still some rough edges

Fixed in 9e7b241. Anything else?

Didn't find anything else, but CI doesn't seem to be happy about your cacheStealingTest even though it works on my machine. Perhaps windows line endings?

listings > cacheStealingTypeCheck.pkl FAILED
    Extra content at line 1:
      ["foo {",
       "  "abcdx"",
       "  "ax"",
       "}"]

odenix · 2024-12-03T21:01:28Z

CI doesn't seem to be happy about your cacheStealingTest

Fixed (forgot to commit the test output file) and added one more commit ("Rename method, add implementation comments").

stackoverflow · 2024-12-04T14:23:31Z

This PR is good to go from my side. You can add your commit comment here or squash the commits yourself. We always squash as we merge anyway so every PR is one commit.

odenix · 2024-12-04T18:28:48Z

Squashed and edited commit message.

Motivation: - simplify implementation of lazy type checking - fix correctness issues of lazy type checking (apple#785) Changes: - implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`) - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - fix apple#785 by executing all type casts between a member's owner and receiver - fix apple#823 by storing owner and receiver directly instead of storing the mutable frame containing them (typeNodeFrame) - remove overrides of VmObject methods that are no longer required - good for Truffle partial evaluation and JVM inlining - revert a85a173 except for added tests - move `VmUtils.setOwner` and `VmUtils.setReceiver` and make them private - these methods aren't generally safe to use Result: - simpler code with greater optimization potential - VmListingOrMapping can now have both a type node and new members - fewer changes to surrounding code - smaller memory footprint - better performance in some cases - fixes apple#785 - fixes apple#823 Potential future optimizations: - avoid lazy type checking overhead for untyped listings/mappings - improve efficiency of forcing a typed listing/mapping - currently, lazy type checking will traverse the parent chain once per member, reducing the performance benefit of shallow-forcing a listing/mapping over evaluating each member individually - avoid creating an intermediate untyped listing/mapping in the following cases: - `new Listing<X> {...}` - amendment of `property: Listing<X>`

odenix · 2024-12-04T19:03:04Z

Added two more tests based on #822 (comment):

listingTypeCheckError9.pkl
mappingTypeCheckError11.pkl

Motivation: - simplify implementation of lazy type checking - fix correctness issues of lazy type checking (#785) Changes: - implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`) - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - fix #785 by executing all type casts between a member's owner and receiver - fix #823 by storing owner and receiver directly instead of storing the mutable frame containing them (typeNodeFrame) - remove overrides of VmObject methods that are no longer required - good for Truffle partial evaluation and JVM inlining - revert a85a173 except for added tests - move `VmUtils.setOwner` and `VmUtils.setReceiver` and make them private - these methods aren't generally safe to use Result: - simpler code with greater optimization potential - VmListingOrMapping can now have both a type node and new members - fewer changes to surrounding code - smaller memory footprint - better performance in some cases - fixes #785 - fixes #823 Potential future optimizations: - avoid lazy type checking overhead for untyped listings/mappings - improve efficiency of forcing a typed listing/mapping - currently, lazy type checking will traverse the parent chain once per member, reducing the performance benefit of shallow-forcing a listing/mapping over evaluating each member individually - avoid creating an intermediate untyped listing/mapping in the following cases: - `new Listing<X> {...}` - amendment of `property: Listing<X>`

PRs #789 and #837 being merged caused a compile error; this fixes them.

odenix marked this pull request as draft November 7, 2024 20:16

odenix force-pushed the lazy-check branch 2 times, most recently from 9d78e92 to 6a793b7 Compare November 7, 2024 20:48

bioball reviewed Nov 11, 2024

View reviewed changes

pkl-core/src/main/java/org/pkl/core/ast/member/ElementOrEntryNode.java Show resolved Hide resolved

odenix force-pushed the lazy-check branch from 724482d to 3c0ca95 Compare November 14, 2024 23:49

bioball previously requested changes Nov 22, 2024

View reviewed changes

odenix force-pushed the lazy-check branch from 2ec4124 to bc10eba Compare November 28, 2024 07:43

odenix marked this pull request as ready for review November 28, 2024 19:34

odenix mentioned this pull request Nov 28, 2024

fix regression where typealiases are not executed in their original context #830

Closed

stackoverflow requested changes Nov 29, 2024

View reviewed changes

odenix force-pushed the lazy-check branch from 3279ad9 to 8f65b68 Compare December 1, 2024 01:41

stackoverflow approved these changes Dec 2, 2024

View reviewed changes

holzensp approved these changes Dec 2, 2024

View reviewed changes

odenix force-pushed the lazy-check branch from 9a5d457 to 84b79b3 Compare December 2, 2024 18:00

odenix force-pushed the lazy-check branch from 9e7b241 to df6bd81 Compare December 3, 2024 21:00

odenix force-pushed the lazy-check branch from df6bd81 to 3abcfff Compare December 4, 2024 18:19

odenix force-pushed the lazy-check branch from 3abcfff to a8957e1 Compare December 4, 2024 19:00

stackoverflow merged commit 1bc473b into apple:main Dec 6, 2024
5 checks passed

bioball mentioned this pull request Dec 19, 2024

Fix compile error #857

Merged

bioball added a commit that referenced this pull request Dec 19, 2024

Fix compile error (#857)

efe1608

PRs #789 and #837 being merged caused a compile error; this fixes them.

		private final @Nullable ListingOrMappingTypeCastNode typeCastNode;
		private final MaterializedFrame typeNodeFrame;

	// This method effectively covers `VmObject receiver` but is implemented in a more
	// efficient way. See:
	// https://www.graalvm.org/22.0/graalvm-as-a-platform/language-implementation-framework/TruffleLibraries/#strategy-2-java-interfaces
	@Specialization(guards = "receiver.getClass() == cachedClass", limit = "99")
	protected Object evalObject(

	protected Object evalDynamic(VirtualFrame frame, @SuppressWarnings("unused") VmDynamic receiver) {
	protected Object evalDynamic(VirtualFrame frame, VmDynamic ignored) {

Simplify lazy type checking of listings/mappings #789

Simplify lazy type checking of listings/mappings #789

Conversation

odenix commented Nov 7, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odenix Nov 11, 2024 • edited Loading

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

odenix Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odenix commented Nov 11, 2024 • edited Loading

bioball commented Nov 12, 2024 • edited Loading

bioball commented Nov 12, 2024

odenix commented Nov 14, 2024

odenix commented Nov 14, 2024 • edited Loading

bioball commented Nov 17, 2024

odenix commented Nov 18, 2024 • edited Loading

bioball commented Nov 20, 2024 • edited Loading

bioball left a 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

Choose a reason for hiding this comment

odenix Nov 27, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

bioball Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

odenix Nov 28, 2024 • edited Loading

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

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

bioball commented Nov 23, 2024

odenix commented Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

odenix Nov 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackoverflow commented Nov 29, 2024

odenix commented Nov 29, 2024 • edited Loading

odenix commented Nov 29, 2024 • edited Loading

stackoverflow left a comment

Choose a reason for hiding this comment

holzensp left a 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

odenix Nov 11, 2024 •

edited

Loading

odenix Nov 11, 2024 •

edited

Loading

odenix commented Nov 11, 2024 •

edited

Loading

bioball commented Nov 12, 2024 •

edited

Loading

odenix commented Nov 14, 2024 •

edited

Loading

odenix commented Nov 18, 2024 •

edited

Loading

bioball commented Nov 20, 2024 •

edited

Loading

odenix Nov 27, 2024 •

edited

Loading

bioball Nov 22, 2024 •

edited

Loading

bioball Nov 22, 2024 •

edited

Loading

odenix Nov 28, 2024 •

edited

Loading

odenix commented Nov 28, 2024 •

edited

Loading

odenix Nov 29, 2024 •

edited

Loading

odenix commented Nov 29, 2024 •

edited

Loading

odenix commented Nov 29, 2024 •

edited

Loading

odenix Dec 2, 2024 •

edited

Loading

odenix Dec 7, 2024 •

edited

Loading

odenix Dec 2, 2024 •

edited

Loading

odenix Dec 2, 2024 •

edited

Loading