Skip to content

Commit

Permalink
Address comments per review
Browse files Browse the repository at this point in the history
* Defer initialization of shared member node, add @child annotations
* Reduce footprint of truffle boundaries
* Revert bugfix when reflecting upon modules with globbed imports
  • Loading branch information
bioball committed May 28, 2024
1 parent 5fba868 commit e2a4604
Show file tree
Hide file tree
Showing 12 changed files with 65 additions and 95 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2211,8 +2211,7 @@ public Object visitReadExpr(ReadExprContext ctx) {
return ReadOrNullNodeGen.create(createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
}
assert tokenType == PklLexer.READ_GLOB;
return ReadGlobNodeGen.create(
language, createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
return ReadGlobNodeGen.create(createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
}

@Override
Expand Down Expand Up @@ -2653,8 +2652,7 @@ private AbstractImportNode doVisitImport(
}
var resolvedUri = resolveImport(importUri, importUriCtx);
if (isGlobImport) {
return new ImportGlobNode(
language, section, moduleInfo.getResolvedModuleKey(), resolvedUri, importUri);
return new ImportGlobNode(section, moduleInfo.getResolvedModuleKey(), resolvedUri, importUri);
}
return new ImportNode(language, section, moduleInfo.getResolvedModuleKey(), resolvedUri);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@
import org.pkl.core.util.GlobResolver.ResolvedGlobElement;

/** Used by {@link ReadGlobNode}. */
public final class ImportGlobElementNode extends ExpressionNode {
public final class ImportGlobMemberBodyNode extends ExpressionNode {
private final VmLanguage language;
private final ResolvedModuleKey currentModule;

public ImportGlobElementNode(
public ImportGlobMemberBodyNode(
SourceSection sourceSection, VmLanguage language, ResolvedModuleKey currentModule) {
super(sourceSection);
this.language = language;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,26 +39,32 @@
@NodeInfo(shortName = "import*")
public class ImportGlobNode extends AbstractImportNode {
private final String globPattern;
private final SharedMemberNode importGlobElementNode;

@Child @LateInit private SharedMemberNode memberNode;
@CompilationFinal @LateInit private VmMapping cachedResult;

public ImportGlobNode(
VmLanguage language,
SourceSection sourceSection,
ResolvedModuleKey currentModule,
URI importUri,
String globPattern) {
super(sourceSection, currentModule, importUri);
this.globPattern = globPattern;
importGlobElementNode =
new SharedMemberNode(
sourceSection,
sourceSection,
"",
language,
new FrameDescriptor(),
new ImportGlobElementNode(sourceSection, language, currentModule));
}

private SharedMemberNode getMemberNode() {
if (memberNode == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
var language = VmLanguage.get(this);
memberNode =
new SharedMemberNode(
sourceSection,
sourceSection,
"",
language,
new FrameDescriptor(),
new ImportGlobMemberBodyNode(sourceSection, language, currentModule));
}
return memberNode;
}

@Override
Expand All @@ -81,9 +87,9 @@ public Object executeGeneric(VirtualFrame frame) {
currentModule.getOriginal(),
currentModule.getUri(),
globPattern);
var builder = new VmObjectBuilder();
var builder = new VmObjectBuilder(resolvedElements.size());
for (var entry : resolvedElements.entrySet()) {
builder.addEntry(entry.getKey(), importGlobElementNode);
builder.addEntry(entry.getKey(), getMemberNode());
}
cachedResult = builder.toMapping(resolvedElements);
return cachedResult;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
package org.pkl.core.ast.expression.unary;

import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.source.SourceSection;
import java.util.Map;
Expand All @@ -26,8 +26,8 @@
import org.pkl.core.util.GlobResolver.ResolvedGlobElement;

/** Used by {@link ReadGlobNode}. */
public class ReadGlobElementNode extends ExpressionNode {
public ReadGlobElementNode(SourceSection sourceSection) {
public class ReadGlobMemberBodyNode extends ExpressionNode {
public ReadGlobMemberBodyNode(SourceSection sourceSection) {
super(sourceSection);
}

Expand All @@ -38,13 +38,13 @@ public Object executeGeneric(VirtualFrame frame) {
return readResource(mapping, path);
}

@TruffleBoundary
private Object readResource(VmObjectLike mapping, String path) {
@SuppressWarnings("unchecked")
var globElements = (Map<String, ResolvedGlobElement>) mapping.getExtraStorage();
var resourceUri = globElements.get(path).getUri();
var resourceUri = VmUtils.getMapValue(globElements, path).getUri();
var resource = VmContext.get(this).getResourceManager().read(resourceUri, this).orElse(null);
if (resource == null) {
CompilerDirectives.transferToInterpreter();
throw exceptionBuilder().evalError("cannotFindResource", resourceUri).build();
}
return resource;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.pkl.core.ast.expression.unary;

import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.FrameDescriptor;
Expand All @@ -32,23 +33,31 @@
import org.pkl.core.runtime.VmObjectBuilder;
import org.pkl.core.util.GlobResolver;
import org.pkl.core.util.GlobResolver.InvalidGlobPatternException;
import org.pkl.core.util.LateInit;

@NodeInfo(shortName = "read*")
public abstract class ReadGlobNode extends AbstractReadNode {
private final SharedMemberNode readGlobElementNode;
private final EconomicMap<String, VmMapping> cachedResults = EconomicMap.create();
@Child @LateInit private SharedMemberNode memberNode;

protected ReadGlobNode(
VmLanguage language, SourceSection sourceSection, ModuleKey currentModule) {
protected ReadGlobNode(SourceSection sourceSection, ModuleKey currentModule) {
super(sourceSection, currentModule);
readGlobElementNode =
new SharedMemberNode(
sourceSection,
sourceSection,
"",
language,
new FrameDescriptor(),
new ReadGlobElementNode(sourceSection));
}

private SharedMemberNode getMemberNode() {
if (memberNode == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
var language = VmLanguage.get(this);
memberNode =
new SharedMemberNode(
sourceSection,
sourceSection,
"",
language,
new FrameDescriptor(),
new ReadGlobMemberBodyNode(sourceSection));
}
return memberNode;
}

@Specialization
Expand Down Expand Up @@ -76,9 +85,9 @@ public Object read(String globPattern) {
currentModule,
currentModule.getUri(),
globPattern);
var builder = new VmObjectBuilder();
var builder = new VmObjectBuilder(resolvedElements.size());
for (var entry : resolvedElements.entrySet()) {
builder.addEntry(entry.getKey(), readGlobElementNode);
builder.addEntry(entry.getKey(), getMemberNode());
}
cachedResult = builder.toMapping(resolvedElements);
cachedResults.put(globPattern, cachedResult);
Expand Down
18 changes: 7 additions & 11 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -225,23 +225,19 @@ public void force(boolean allowUndefinedValues) {
}
}

@TruffleBoundary
public VmMapping toMapping() {
var builder = new VmObjectBuilder(keyOrder.size());
for (var key : keyOrder) {
var value = map.get(key);
assert value != null;
builder.addEntry(key, value);
var builder = new VmObjectBuilder(getLength());
for (var entry : this) {
builder.addEntry(VmUtils.getKey(entry), VmUtils.getValue(entry));
}
return builder.toMapping();
}

@TruffleBoundary
public VmDynamic toDynamic() {
var builder = new VmObjectBuilder(keyOrder.size());
for (var key : keyOrder) {
var value = map.get(key);
assert value != null;
var builder = new VmObjectBuilder(getLength());
for (var entry : this) {
var key = VmUtils.getKey(entry);
var value = VmUtils.getValue(entry);
if (key instanceof String) {
builder.addProperty(Identifier.get((String) key), value);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ public final class VmObjectBuilder {
private final EconomicMap<Object, ObjectMember> members;
private int elementCount = 0;

public VmObjectBuilder() {
members = EconomicMaps.create();
}

public VmObjectBuilder(int initialSize) {
members = EconomicMaps.create(initialSize);
}
Expand Down
4 changes: 2 additions & 2 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmTyped.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import org.pkl.core.Composite;
import org.pkl.core.PModule;
import org.pkl.core.PObject;
import org.pkl.core.ast.expression.unary.AbstractImportNode;
import org.pkl.core.ast.expression.unary.ImportNode;
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.util.EconomicMaps;
import org.pkl.core.util.LateInit;
Expand Down Expand Up @@ -105,7 +105,7 @@ public VmMap getImports() {
assert memberNode != null; // import is never a constant
builder.add(
member.getName().toString(),
((AbstractImportNode) memberNode.getBodyNode()).getImportUri().toString());
((ImportNode) memberNode.getBodyNode()).getImportUri().toString());
}
}
return builder.build();
Expand Down
5 changes: 5 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -844,4 +844,9 @@ public static int findSlot(VirtualFrame frame, Object identifier) {
public static int findAuxiliarySlot(VirtualFrame frame, Object identifier) {
return frame.getFrameDescriptor().getAuxiliarySlots().getOrDefault(identifier, -1);
}

@TruffleBoundary
public static <K, V> V getMapValue(Map<K, V> map, K key) {
return map.get(key);
}
}
25 changes: 1 addition & 24 deletions pkl-core/src/main/java/org/pkl/core/stdlib/base/MapNodes.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@
import com.oracle.truffle.api.nodes.LoopNode;
import java.util.Map;
import org.pkl.core.ast.lambda.*;
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.runtime.*;
import org.pkl.core.stdlib.ExternalMethod0Node;
import org.pkl.core.stdlib.ExternalMethod1Node;
import org.pkl.core.stdlib.ExternalMethod2Node;
import org.pkl.core.stdlib.ExternalPropertyNode;
import org.pkl.core.util.EconomicMaps;
import org.pkl.core.util.MutableReference;

public final class MapNodes {
Expand Down Expand Up @@ -238,28 +236,7 @@ protected VmMap eval(VmMap self) {
public abstract static class toDynamic extends ExternalMethod0Node {
@Specialization
protected VmDynamic eval(VmMap self) {
var members = EconomicMaps.<Object, ObjectMember>create(self.getLength());

for (var entry : self) {
var key = VmUtils.getKey(entry);

if (key instanceof String string) {
var name = Identifier.get(string);
EconomicMaps.put(
members,
name,
VmUtils.createSyntheticObjectProperty(name, "", VmUtils.getValue(entry)));
} else {
EconomicMaps.put(
members, key, VmUtils.createSyntheticObjectEntry("", VmUtils.getValue(entry)));
}
}

return new VmDynamic(
VmUtils.createEmptyMaterializedFrame(),
BaseModule.getDynamicClass().getPrototype(),
members,
0);
return self.toDynamic();
}
}

Expand Down

This file was deleted.

This file was deleted.

0 comments on commit e2a4604

Please sign in to comment.