Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register type definitions lazily #11938

Merged
merged 10 commits into from
Jan 1, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.enso.interpreter.runtime.data.vector.ArrayLikeHelpers;
import org.enso.interpreter.runtime.library.dispatch.TypesLibrary;
import org.enso.interpreter.runtime.scope.ModuleScope;
import org.enso.interpreter.runtime.util.CachingSupplier;
import org.enso.pkg.QualifiedName;

@ExportLibrary(TypesLibrary.class)
Expand Down Expand Up @@ -217,21 +218,25 @@ public void generateGetters(EnsoLanguage language) {
var roots = AtomConstructor.collectFieldAccessors(language, this);
roots.forEach(
(name, node) -> {
var schemaBldr =
FunctionSchema.newBuilder()
.argumentDefinitions(
new ArgumentDefinition(
0,
Constants.Names.SELF_ARGUMENT,
null,
null,
ArgumentDefinition.ExecutionMode.EXECUTE));
if (isProjectPrivate) {
schemaBldr.projectPrivate();
}
var funcSchema = schemaBldr.build();
var f = new Function(node.getCallTarget(), null, funcSchema);
definitionScope.registerMethod(this, name, f);
var functionSupplier =
CachingSupplier.wrap(
() -> {
var schemaBldr =
FunctionSchema.newBuilder()
.argumentDefinitions(
new ArgumentDefinition(
0,
Constants.Names.SELF_ARGUMENT,
null,
null,
ArgumentDefinition.ExecutionMode.EXECUTE));
if (isProjectPrivate) {
schemaBldr.projectPrivate();
}
var funcSchema = schemaBldr.build();
return new Function(node.getCallTarget(), null, funcSchema);
});
definitionScope.registerMethod(this, name, functionSupplier);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.TreeMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Supplier;
import org.enso.compiler.context.LocalScope;
import org.enso.interpreter.EnsoLanguage;
import org.enso.interpreter.node.ExpressionNode;
Expand All @@ -33,6 +34,7 @@
import org.enso.interpreter.runtime.data.Type;
import org.enso.interpreter.runtime.library.dispatch.TypesLibrary;
import org.enso.interpreter.runtime.scope.ModuleScope;
import org.enso.interpreter.runtime.util.CachingSupplier;
import org.enso.pkg.QualifiedName;

/**
Expand All @@ -47,11 +49,12 @@ public final class AtomConstructor extends EnsoObject {
private final Module definitionModule;
private final boolean builtin;
private @CompilerDirectives.CompilationFinal Atom cachedInstance;
private @CompilerDirectives.CompilationFinal Function constructorFunction;
private @CompilerDirectives.CompilationFinal(dimensions = 1) String[] fieldNames;
private @CompilerDirectives.CompilationFinal Supplier<Function> constructorFunction;
private @CompilerDirectives.CompilationFinal Function accessor;

private final Lock layoutsLock = new ReentrantLock();
private @CompilerDirectives.CompilationFinal Layout boxedLayout;
private @CompilerDirectives.CompilationFinal Supplier<Layout> boxedLayout;
private Layout[] unboxingLayouts = new Layout[0];

private final Type type;
Expand Down Expand Up @@ -90,13 +93,39 @@ public AtomConstructor(String name, Module definitionModule, Type type, boolean
* @return {@code true} if {@link #initializeFields} method has already been called
*/
public boolean isInitialized() {
return constructorFunction != null;
return accessor != null;
}

boolean isBuiltin() {
return builtin;
}

/**
* Building blocks required for initialization of the atom constructor.
*
* @param section the source section
* @param localScope the description of the local scope
* @param assignments the expressions that evaluate and assign constructor arguments to local vars
* @param varReads the expressions that read field values from local vars
* @param annotations the list of attached annotations
* @param args the list of argument definitions
*/
public record InitializationParts(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new record have to be public? Because the record defines not just public constructor, but also public getters. I do not think we want people around the code base to call the getters.

Could we rather follow this example by @Akirathan where Pavel introduced FunctionSchema.Builder?

SourceSection section,
LocalScope localScope,
ExpressionNode[] assignments,
ExpressionNode[] varReads,
Annotation[] annotations,
ArgumentDefinition[] args) {}

/**
* The result of this atom constructor initialization.
*
* @param constructorFunction the atom constructor function
* @param layout the atom layout
*/
private record InitializationResult(Function constructorFunction, Layout layout) {}

/**
* Generates a constructor function for this {@link AtomConstructor}. Note that such manually
* constructed argument definitions must not have default arguments.
Expand All @@ -106,49 +135,62 @@ boolean isBuiltin() {
public AtomConstructor initializeFields(
EnsoLanguage language, ModuleScope.Builder scopeBuilder, ArgumentDefinition... args) {
ExpressionNode[] reads = new ExpressionNode[args.length];
String[] fieldNames = new String[args.length];
for (int i = 0; i < args.length; i++) {
reads[i] = ReadArgumentNode.build(i, null);
fieldNames[i] = args[i].getName();
}
return initializeFields(
language,
null,
LocalScope.empty(),
scopeBuilder,
new ExpressionNode[0],
reads,
new Annotation[0],
args);

var parts =
new InitializationParts(
null, LocalScope.empty(), new ExpressionNode[0], reads, new Annotation[0], args);
return initializeFields(language, scopeBuilder, CachingSupplier.forValue(parts), fieldNames);
}

/**
* Sets the fields of this {@link AtomConstructor} and generates a constructor function.
*
* @param localScope a description of the local scope
* @param assignments the expressions that evaluate and assign constructor arguments to local vars
* @param language the language implementation
* @param scopeBuilder the module scope's builder where the accessor should be registered at
* @param varReads the expressions that read field values from local vars
* @param initializationPartsSupplier the function supplying the parts required for initialization
* @param argumentsLength the number of the arguments
* @return {@code this}, for convenience
*/
public AtomConstructor initializeFields(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can initializeFields be private now, when there is the builder?

EnsoLanguage language,
SourceSection section,
LocalScope localScope,
ModuleScope.Builder scopeBuilder,
ExpressionNode[] assignments,
ExpressionNode[] varReads,
Annotation[] annotations,
ArgumentDefinition... args) {
Supplier<InitializationParts> initializationPartsSupplier,
String[] fieldNames) {
CompilerDirectives.transferToInterpreterAndInvalidate();
assert boxedLayout == null : "Don't initialize twice: " + this.name;
if (args.length == 0) {
assert accessor == null : "Don't initialize twice: " + this.name;
this.fieldNames = fieldNames;
if (fieldNames.length == 0) {
cachedInstance = BoxingAtom.singleton(this);
} else {
cachedInstance = null;
}
boxedLayout = Layout.createBoxed(args);
CachingSupplier<InitializationResult> initializationResultSupplier =
CachingSupplier.wrap(
() -> {
var parts = initializationPartsSupplier.get();
var constructorFunction =
buildConstructorFunction(
language,
parts.section,
parts.localScope,
scopeBuilder,
parts.assignments,
parts.varReads,
parts.annotations,
parts.args);
var layout = Layout.createBoxed(parts.args);
return new InitializationResult(constructorFunction, layout);
});
this.boxedLayout =
initializationResultSupplier.map(initializationResult -> initializationResult.layout);
this.constructorFunction =
buildConstructorFunction(
language, section, localScope, scopeBuilder, assignments, varReads, annotations, args);
initializationResultSupplier.map(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting use of the new map function. The alternative would be to have a single field Supplier<InitializationResult> stored in the AtomConstructor. I seem to like it a bit more - shows the "fixed" vs. the lazily computed part of the structure.

initializationResult -> initializationResult.constructorFunction);
this.accessor = generateQualifiedAccessor(language, scopeBuilder);
return this;
}
Expand Down Expand Up @@ -244,7 +286,7 @@ public ModuleScope getDefinitionScope() {
* @return the number of args expected by the constructor.
*/
public int getArity() {
return constructorFunction.getSchema().getArgumentsCount();
return fieldNames.length;
}

/**
Expand Down Expand Up @@ -278,8 +320,9 @@ public String toString() {
*
* @return the constructor function of this constructor.
*/
@TruffleBoundary
Copy link
Member

@JaroslavTulach JaroslavTulach Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slowing execution down.

obrazek

This method needs to be inlined, it cannot be @TruffleBoundary. The IGV graph has been generated by:

sbt:runtime-benchmarks> withDebug --dumpGraphs benchOnly -- AtomBenchmarks.benchGenerateList

public Function getConstructorFunction() {
return constructorFunction;
return constructorFunction.get();
}

/**
Expand Down Expand Up @@ -323,9 +366,10 @@ public static Map<String, RootNode> collectFieldAccessors(EnsoLanguage language,
// take just the first one.
var moduleScope = constructors.iterator().next().getDefinitionScope();
for (var cons : constructors) {
for (var field : cons.getFields()) {
var items = names.computeIfAbsent(field.getName(), (k) -> new ArrayList<>());
items.add(new GetFieldWithMatchNode.GetterPair(cons, field.getPosition()));
final var fieldNames = cons.getFieldNames();
for (var i = 0; i < fieldNames.length; i++) {
var items = names.computeIfAbsent(fieldNames[i], (k) -> new ArrayList<>());
items.add(new GetFieldWithMatchNode.GetterPair(cons, i));
}
}
for (var entry : names.entrySet()) {
Expand All @@ -342,11 +386,10 @@ public static Map<String, RootNode> collectFieldAccessors(EnsoLanguage language,
}
} else if (constructors.size() == 1) {
var cons = constructors.toArray(AtomConstructor[]::new)[0];
for (var field : cons.getFields()) {
var node =
new GetFieldNode(
language, field.getPosition(), type, field.getName(), cons.getDefinitionScope());
roots.put(field.getName(), node);
final var fieldNames = cons.getFieldNames();
for (var i = 0; i < fieldNames.length; i++) {
var node = new GetFieldNode(language, i, type, fieldNames[i], cons.getDefinitionScope());
roots.put(fieldNames[i], node);
}
}
return roots;
Expand All @@ -356,8 +399,9 @@ final Layout[] getUnboxingLayouts() {
return unboxingLayouts;
}

@TruffleBoundary
final Layout getBoxedLayout() {
return boxedLayout;
return boxedLayout.get();
}

/**
Expand Down Expand Up @@ -448,7 +492,16 @@ public QualifiedName getQualifiedTypeName() {
* @return the fields defined by this constructor.
*/
public ArgumentDefinition[] getFields() {
return constructorFunction.getSchema().getArgumentInfos();
return getConstructorFunction().getSchema().getArgumentInfos();
}

/**
* Names of this constructor fields.
*
* @return the field names defined by this constructor.
*/
public String[] getFieldNames() {
return fieldNames;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.enso.interpreter.runtime.util;

import com.oracle.truffle.api.CompilerDirectives;
import java.util.function.Function;
import java.util.function.Supplier;

public final class CachingSupplier<T> implements Supplier<T> {
Expand Down Expand Up @@ -55,6 +56,17 @@ public T get() {
}
}

/**
* Transform the result of this supplier by applying a mapping function {@code f}.
*
* @param f the mapping function
* @return the supplier providing the value with the mapping function applied
* @param <R> the result type
*/
public <R> CachingSupplier<R> map(Function<T, R> f) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "tendency towards functional style" is clear...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, on the other hand, really like it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are benchmark regressions and the only change that doesn't seem straightforward to me is this map method. But of course, I may be biased...

return wrap(() -> f.apply(get()));
}

/**
* Returns a supplier that always returns {@code null} when its {@link Supplier#get()} method is
* called.
Expand Down
Loading
Loading