Skip to content

Commit

Permalink
Simplify VmListingOrMapping data structure and code
Browse files Browse the repository at this point in the history
Motivation:
As part of implementing lazy type checking of listings and mappings,
major modifications were made to VmListingOrMapping and its subclasses.
I believe a simpler implementation is possible and desirable.

Changes:
- Implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`).
  This is the key idea.
- 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
- remove overrides of VmObject methods that are no longer required
  - having a single implementation simplifies code and helps JITs

Result:
- simpler code that will be easier to optimize
  - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR).
- smaller memory footprint
- better performance in some cases
- fixes apple#785

Examples for potential future optimizations:
- avoid type check overhead for untyped listings/mappings
- make forcing a typed listing/mapping more efficient
  - currently, the parent chain is traversed once per member for type checking,
    which nullifies much of the performance advantage that shallow-forcing an object
    has over computing each value individually
- avoid creating an intermediate untyped listing/mapping in the following cases:
  - new Listing<X> {...}
  - `property: Listing<X>` is amended
  • Loading branch information
odenix committed Nov 14, 2024
1 parent 79f3b06 commit bbd1b63
Show file tree
Hide file tree
Showing 21 changed files with 231 additions and 276 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,7 @@ private ObjectMember doVisitObjectElement(ObjectElementContext ctx) {
member.initConstantValue(constantNode);
} else {
member.initMemberNode(
new UntypedObjectMemberNode(
ElementOrEntryNodeGen.create(
language, scope.buildFrameDescriptor(), member, elementNode));
}

Expand Down Expand Up @@ -1278,7 +1278,7 @@ private Function<EntryScope, Pair<ExpressionNode, ObjectMember>> objectMemberIns
member.initConstantValue(constantNode);
} else {
member.initMemberNode(
new UntypedObjectMemberNode(
ElementOrEntryNodeGen.create(
language, scope.buildFrameDescriptor(), member, valueNode));
}
} else { // ["key"] { ... }
Expand All @@ -1287,7 +1287,7 @@ private Function<EntryScope, Pair<ExpressionNode, ObjectMember>> objectMemberIns
objectBodyCtxs,
new ReadSuperEntryNode(unavailableSourceSection(), new GetMemberKeyNode()));
member.initMemberNode(
new UntypedObjectMemberNode(
ElementOrEntryNodeGen.create(
language, scope.buildFrameDescriptor(), member, objectBody));
}

Expand Down Expand Up @@ -2446,6 +2446,7 @@ public UnresolvedTypeNode visitDeclaredType(DeclaredTypeContext ctx) {

return new UnresolvedTypeNode.Parameterized(
createSourceSection(ctx),
language,
doVisitTypeName(idCtx),
argCtx.ts.stream().map(this::visitType).toArray(UnresolvedTypeNode[]::new));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ private void addMembers(VirtualFrame frame, VmObject parent, ObjectData data) {
var callTarget = member.getCallTarget();
value = callTarget.call(parent, owner, key);
}
owner.setCachedValue(key, value, member);
owner.setCachedValue(key, value);
}

frame.setAuxiliarySlot(customThisSlot, value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public Object executeGeneric(VirtualFrame frame) {

if (result == null) {
result = callNode.call(objReceiver, owner, property.getName());
objReceiver.setCachedValue(property, result, property);
objReceiver.setCachedValue(property, result);
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,12 @@ private ExpressionNode doResolve(VirtualFrame frame) {
if (member != null) {
var constantValue = member.getConstantValue();
if (constantValue != null) {
baseModule.setCachedValue(variableName, constantValue, member);
baseModule.setCachedValue(variableName, constantValue);
return new ConstantValueNode(sourceSection, constantValue);
}

var computedValue = member.getCallTarget().call(baseModule, baseModule);
baseModule.setCachedValue(variableName, computedValue, member);
baseModule.setCachedValue(variableName, computedValue);
return new ConstantValueNode(sourceSection, computedValue);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.pkl.core.ast.member;

import com.oracle.truffle.api.dsl.Executed;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.FrameDescriptor;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.nodes.IndirectCallNode;
import org.pkl.core.ast.ExpressionNode;
import org.pkl.core.ast.expression.primary.GetReceiverNode;
import org.pkl.core.runtime.VmDynamic;
import org.pkl.core.runtime.VmLanguage;
import org.pkl.core.runtime.VmListing;
import org.pkl.core.runtime.VmMapping;
import org.pkl.core.runtime.VmUtils;
import org.pkl.core.util.Nullable;

public abstract class ElementOrEntryNode extends RegularMemberNode {
@Child @Executed protected ExpressionNode receiverNode = new GetReceiverNode();
@Child private IndirectCallNode callNode = IndirectCallNode.create();

protected ElementOrEntryNode(
@Nullable VmLanguage language,
FrameDescriptor descriptor,
ObjectMember member,
ExpressionNode bodyNode) {

super(language, descriptor, member, bodyNode);
}

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

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

@Specialization
protected Object evalDynamic(VirtualFrame frame, @SuppressWarnings("unused") VmDynamic receiver) {
return executeBody(frame);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ protected VmTyped getImport(
var result = module.getCachedValue(importName);
if (result == null) {
result = callNode.call(member.getCallTarget(), module, module, importName);
module.setCachedValue(importName, result, member);
module.setCachedValue(importName, result);
}
return (VmTyped) result;
}
Expand All @@ -94,7 +94,7 @@ protected VmTyped getImport(
var result = module.getCachedValue(typeName);
if (result == null) {
result = callNode.call(member.getCallTarget(), module, module, typeName);
module.setCachedValue(typeName, result, member);
module.setCachedValue(typeName, result);
}
return result;
}
Expand Down
61 changes: 35 additions & 26 deletions pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -1414,16 +1414,25 @@ protected boolean isParametric() {
}

public static final class ListingTypeNode extends ListingOrMappingTypeNode {
public ListingTypeNode(SourceSection sourceSection, TypeNode valueTypeNode) {
super(sourceSection, null, valueTypeNode);
public ListingTypeNode(
SourceSection sourceSection, VmLanguage language, TypeNode valueTypeNode) {
super(sourceSection, language, null, valueTypeNode);
}

@Override
public Object execute(VirtualFrame frame, Object value) {
if (!(value instanceof VmListing vmListing)) {
throw typeMismatch(value, BaseModule.getListingClass());
}
return doTypeCast(frame, vmListing);
if (vmListing.valueTypeIsSubtypeOf(valueTypeNode)) {
return vmListing;
}
return new VmListing(
frame.materialize(),
vmListing,
EconomicMaps.emptyMap(),
vmListing.getLength(),
getValueTypeCastNode());
}

@Override
Expand Down Expand Up @@ -1466,9 +1475,12 @@ protected boolean acceptTypeNode(TypeNodeConsumer consumer) {

public static final class MappingTypeNode extends ListingOrMappingTypeNode {
public MappingTypeNode(
SourceSection sourceSection, TypeNode keyTypeNode, TypeNode valueTypeNode) {
SourceSection sourceSection,
VmLanguage language,
TypeNode keyTypeNode,
TypeNode valueTypeNode) {

super(sourceSection, keyTypeNode, valueTypeNode);
super(sourceSection, language, keyTypeNode, valueTypeNode);
}

@Override
Expand All @@ -1478,7 +1490,11 @@ public Object execute(VirtualFrame frame, Object value) {
}
// execute type checks on mapping keys
doEagerCheck(frame, vmMapping, false, true);
return doTypeCast(frame, vmMapping);
if (vmMapping.valueTypeIsSubtypeOf(valueTypeNode)) {
return vmMapping;
}
return new VmMapping(
frame.materialize(), vmMapping, EconomicMaps.emptyMap(), getValueTypeCastNode());
}

@Override
Expand Down Expand Up @@ -1526,17 +1542,22 @@ protected boolean acceptTypeNode(TypeNodeConsumer consumer) {
}

public abstract static class ListingOrMappingTypeNode extends ObjectSlotTypeNode {
private final VmLanguage language;
@Child protected @Nullable TypeNode keyTypeNode;
@Child protected TypeNode valueTypeNode;
@Child @Nullable protected ListingOrMappingTypeCastNode listingOrMappingTypeCastNode;
@Child @Nullable protected ListingOrMappingTypeCastNode valueTypeCastNode;

private final boolean skipKeyTypeChecks;
private final boolean skipValueTypeChecks;

protected ListingOrMappingTypeNode(
SourceSection sourceSection, @Nullable TypeNode keyTypeNode, TypeNode valueTypeNode) {
SourceSection sourceSection,
VmLanguage language,
@Nullable TypeNode keyTypeNode,
TypeNode valueTypeNode) {

super(sourceSection);
this.language = language;
this.keyTypeNode = keyTypeNode;
this.valueTypeNode = valueTypeNode;

Expand All @@ -1556,17 +1577,14 @@ public TypeNode getValueTypeNode() {
return valueTypeNode;
}

protected ListingOrMappingTypeCastNode getListingOrMappingTypeCastNode() {
if (listingOrMappingTypeCastNode == null) {
protected ListingOrMappingTypeCastNode getValueTypeCastNode() {
if (valueTypeCastNode == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
listingOrMappingTypeCastNode =
valueTypeCastNode =
new ListingOrMappingTypeCastNode(
VmLanguage.get(this),
getRootNode().getFrameDescriptor(),
valueTypeNode,
getRootNode().getName());
language, new FrameDescriptor(), valueTypeNode, getRootNode().getName());
}
return listingOrMappingTypeCastNode;
return valueTypeCastNode;
}

// either (if defaultMemberValue != null):
Expand Down Expand Up @@ -1647,15 +1665,6 @@ public final Object createDefaultValue(
EconomicMaps.of(Identifier.DEFAULT, defaultMember));
}

protected <T extends VmListingOrMapping<T>> T doTypeCast(VirtualFrame frame, T original) {
// optimization: don't create new object if the original already has the same typecheck, or if
// this typecheck is a no-op.
if (isNoopTypeCheck() || original.hasSameChecksAs(valueTypeNode)) {
return original;
}
return original.withCheckedMembers(getListingOrMappingTypeCastNode(), frame.materialize());
}

protected void doEagerCheck(VirtualFrame frame, VmObject object) {
doEagerCheck(frame, object, skipKeyTypeChecks, skipValueTypeChecks);
}
Expand Down Expand Up @@ -1700,7 +1709,7 @@ protected void doEagerCheck(
var callTarget = member.getCallTarget();
memberValue = callTarget.call(object, owner, memberKey);
}
object.setCachedValue(memberKey, memberValue, member);
object.setCachedValue(memberKey, memberValue);
}
valueTypeNode.executeEagerly(frame, memberValue);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,14 +197,17 @@ public TypeNode execute(VirtualFrame frame) {
}

public static final class Parameterized extends UnresolvedTypeNode {
private final VmLanguage language;
@Child private ExpressionNode resolveTypeNode;
@Children private final UnresolvedTypeNode[] typeArgumentNodes;

public Parameterized(
SourceSection sourceSection,
VmLanguage language,
ExpressionNode resolveTypeNode,
UnresolvedTypeNode[] typeArgumentNodes) {
super(sourceSection);
this.language = language;
this.resolveTypeNode = resolveTypeNode;
this.typeArgumentNodes = typeArgumentNodes;
}
Expand Down Expand Up @@ -238,12 +241,13 @@ public TypeNode execute(VirtualFrame frame) {
}

if (clazz.isListingClass()) {
return new ListingTypeNode(sourceSection, typeArgumentNodes[0].execute(frame));
return new ListingTypeNode(sourceSection, language, typeArgumentNodes[0].execute(frame));
}

if (clazz.isMappingClass()) {
return new MappingTypeNode(
sourceSection,
language,
typeArgumentNodes[0].execute(frame),
typeArgumentNodes[1].execute(frame));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public UnmodifiableEconomicMap<Object, ObjectMember> getMembers() {

@Override
@TruffleBoundary
public void setCachedValue(Object key, Object value, ObjectMember objectMember) {
public void setCachedValue(Object key, Object value) {
throw new VmExceptionBuilder()
.bug("Class `VmFunction` does not support method `setCachedValue()`.")
.build();
Expand Down
31 changes: 4 additions & 27 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmListing.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@
import com.oracle.truffle.api.frame.MaterializedFrame;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import org.graalvm.collections.UnmodifiableEconomicMap;
import org.pkl.core.ast.member.ListingOrMappingTypeCastNode;
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.util.EconomicMaps;
import org.pkl.core.util.Nullable;

public final class VmListing extends VmListingOrMapping<VmListing> {
public final class VmListing extends VmListingOrMapping {
private static final class EmptyHolder {
private static final VmListing EMPTY =
new VmListing(
Expand All @@ -47,7 +46,7 @@ public VmListing(
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
int length) {
super(enclosingFrame, Objects.requireNonNull(parent), members, null, null, null);
super(enclosingFrame, parent, members, null);
this.length = length;
}

Expand All @@ -56,16 +55,8 @@ public VmListing(
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
int length,
@Nullable VmListing delegate,
ListingOrMappingTypeCastNode typeCheckNode,
MaterializedFrame typeNodeFrame) {
super(
enclosingFrame,
Objects.requireNonNull(parent),
members,
delegate,
typeCheckNode,
typeNodeFrame);
ListingOrMappingTypeCastNode typeCastNode) {
super(enclosingFrame, parent, members, typeCastNode);
this.length = length;
}

Expand Down Expand Up @@ -117,20 +108,6 @@ public <T> T accept(VmValueConverter<T> converter, Iterable<Object> path) {
return converter.convertListing(this, path);
}

@Override
public VmListing withCheckedMembers(
ListingOrMappingTypeCastNode typeCheckNode, MaterializedFrame typeNodeFrame) {

return new VmListing(
getEnclosingFrame(),
Objects.requireNonNull(parent),
members,
length,
this,
typeCheckNode,
typeNodeFrame);
}

@Override
@TruffleBoundary
public boolean equals(@Nullable Object obj) {
Expand Down
Loading

0 comments on commit bbd1b63

Please sign in to comment.