Skip to content

[GR-63234] Make mapping between OptionDescriptors and OptionContainers canonical #10903

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.

Sign up for GitHub

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

Merged
graalvmbot merged 1 commit into from
Mar 26, 2025
Merged

[GR-63234] Make mapping between OptionDescriptors and OptionContainers canonical #10903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions compiler/mx.compiler/mx_compiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,17 +1321,15 @@ def add_serviceprovider(service, provider, version):
if self.isTest:
mx.warn('@Option defined in test code will be ignored: ' + arcname)
else:
# Need to create service files for the providers of the
# jdk.internal.vm.ci.options.Options service created by
# jdk.internal.vm.ci.options.processor.OptionProcessor.
version_prefix = 'META-INF/versions/'
if arcname.startswith(version_prefix):
# If OptionDescriptor is version-specific, get version
# If OptionDescriptors is version-specific, get version
# from arcname and adjust arcname to non-version form
version, _, arcname = arcname[len(version_prefix):].partition('/')
else:
version = None
provider = arcname[:-len('.class'):].replace('/', '.')

provider = arcname[0:-len('.class')].replace("/", ".")
service = 'jdk.graal.compiler.options.OptionDescriptors'
add_serviceprovider(service, provider, version)
return False
Expand Down
11 changes: 9 additions & 2 deletions ...iler/src/jdk.graal.compiler.libgraal/src/jdk/graal/compiler/libgraal/LibGraalFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,12 +232,19 @@ void afterAnalysis(AfterAnalysisAccess access) {
Map<String, String> modules = libgraalLoader.getClassModuleMap();
for (OptionKey<?> option : options) {
OptionDescriptor descriptor = option.getDescriptor();
if (descriptor.isServiceLoaded()) {
if (descriptor != null) {
GraalError.guarantee(access.isReachable(option.getClass()), "%s", option.getClass());
GraalError.guarantee(access.isReachable(descriptor.getClass()), "%s", descriptor.getClass());

String name = option.getName();
OptionsParser.libgraalOptions.descriptors().put(name, descriptor);
OptionDescriptor conflict = OptionsParser.libgraalOptions.descriptors().put(name, descriptor);
if (conflict != null) {
throw new GraalError("Duplicate option names for %s.% and %s.%s",
descriptor.getDeclaringClass().getName(),
descriptor.getFieldName(),
conflict.getDeclaringClass().getName(),
conflict.getFieldName());
}

String module = modules.get(descriptor.getDeclaringClass().getName());
if (module.contains("enterprise")) {
Expand Down
273 changes: 119 additions & 154 deletions ...dk.graal.compiler.processor/src/jdk/graal/compiler/options/processor/OptionProcessor.java
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 3 additions & 6 deletions .../src/jdk.graal.compiler.processor/src/jdk/graal/compiler/processor/AbstractProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ public static <T> T getAnnotationValue(AnnotationMirror annotation, String name,
}

if (valueMethod == null) {
return null;
throw new NoSuchElementException(annotation.getAnnotationType() + " has no element named " + name);
}

AnnotationValue value = annotation.getElementValues().get(valueMethod);
Expand All @@ -292,11 +292,8 @@ public static <T> T getAnnotationValue(AnnotationMirror annotation, String name,
public static <T> List<T> getAnnotationValueList(AnnotationMirror annotation, String name, Class<T> componentType) {
List<? extends AnnotationValue> values = getAnnotationValue(annotation, name, List.class);
List<T> result = new ArrayList<>();

if (values != null) {
for (AnnotationValue value : values) {
result.add(componentType.cast(value.getValue()));
}
for (AnnotationValue value : values) {
result.add(componentType.cast(value.getValue()));
}
return result;
}
Expand Down
30 changes: 12 additions & 18 deletions ...jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ReflectionOptionDescriptors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@
import java.util.Map;
import java.util.Properties;

import jdk.graal.compiler.options.OptionsContainer;
import org.graalvm.collections.EconomicMap;
import org.graalvm.collections.MapCursor;
import jdk.graal.compiler.options.Option;
import jdk.graal.compiler.options.OptionDescriptor;
import jdk.graal.compiler.options.OptionDescriptors;
Expand Down Expand Up @@ -76,31 +76,26 @@ public static EconomicMap<String, String> extractEntries(Properties properties,

private final EconomicMap<String, OptionDescriptor> descriptors = EconomicMap.create();

// SVM expects a default constructor here.
public ReflectionOptionDescriptors() {
private final OptionsContainer container;

@Override
public OptionsContainer getContainer() {
return container;
}

public ReflectionOptionDescriptors(Class<?> declaringClass, String... fieldsAndHelp) {
this.container = OptionsContainer.asContainer(declaringClass);
assert fieldsAndHelp.length % 2 == 0;
for (int i = 0; i < fieldsAndHelp.length; i += 2) {
String fieldName = fieldsAndHelp[i];
String help = fieldsAndHelp[i + 1];
addOption(declaringClass, fieldName, help);
}
}

public ReflectionOptionDescriptors(Class<?> declaringClass, EconomicMap<String, String> fieldsAndHelp) {
MapCursor<String, String> cursor = fieldsAndHelp.getEntries();
while (cursor.advance()) {
String fieldName = cursor.getKey();
String help = cursor.getValue();
addOption(declaringClass, fieldName, help);
addOption(fieldName, help);
}
}

private void addOption(Class<?> declaringClass, String fieldName, String help) {
private void addOption(String fieldName, String help) {
try {
Field f = declaringClass.getDeclaredField(fieldName);
Field f = container.getDeclaringClass().getDeclaredField(fieldName);
if (!OptionKey.class.isAssignableFrom(f.getType())) {
throw new IllegalArgumentException(String.format("Option field must be of type %s: %s", OptionKey.class.getName(), f));
}
Expand All @@ -109,14 +104,13 @@ private void addOption(Class<?> declaringClass, String fieldName, String help) {
}
f.setAccessible(true);
Type declaredType = f.getAnnotatedType().getType();
if (!(declaredType instanceof ParameterizedType)) {
if (!(declaredType instanceof ParameterizedType pt)) {
throw new IllegalArgumentException(String.format("Option field must have a parameterized type: %s", f));
}
ParameterizedType pt = (ParameterizedType) declaredType;
Type[] actualTypeArguments = pt.getActualTypeArguments();
assert actualTypeArguments.length == 1;
Class<?> optionValueType = (Class<?>) actualTypeArguments[0];
descriptors.put(fieldName, OptionDescriptor.create(fieldName, OptionType.Debug, optionValueType, help, declaringClass, fieldName, (OptionKey<?>) f.get(null)));
descriptors.put(fieldName, OptionDescriptor.create(fieldName, OptionType.Debug, optionValueType, help, container, fieldName, (OptionKey<?>) f.get(null)));
} catch (IllegalAccessException | NoSuchFieldException e) {
throw new IllegalArgumentException(e);
}
Expand Down
4 changes: 2 additions & 2 deletions ...iler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/CompileTheWorld.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import jdk.graal.compiler.options.OptionsContainer;
import org.graalvm.collections.EconomicMap;

import com.oracle.truffle.runtime.hotspot.libgraal.LibGraal;
Expand Down Expand Up @@ -146,7 +146,7 @@ public static EconomicMap<OptionKey<?>, Object> parseOptions(String options) {
for (String optionSetting : options.split("\\s+|#")) {
OptionsParser.parseOptionSettingTo(optionSetting, optionSettings);
}
ServiceLoader<OptionDescriptors> loader = ServiceLoader.load(OptionDescriptors.class, OptionDescriptors.class.getClassLoader());
Iterable<OptionDescriptors> loader = OptionsContainer.load(OptionDescriptors.class.getClassLoader());
OptionsParser.parseOptions(optionSettings, values, loader);
}
if (!values.containsKey(HighTier.Options.Inline)) {
Expand Down
57 changes: 30 additions & 27 deletions compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/options/OptionDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
*/
package jdk.graal.compiler.options;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

/**
Expand All @@ -40,7 +38,7 @@ public final class OptionDescriptor {
private final String help;
private final List<String> extraHelp;
private final OptionKey<?> optionKey;
private final Class<?> declaringClass;
private final OptionsContainer container;
private final String fieldName;
private final OptionStability stability;
private final boolean deprecated;
Expand All @@ -52,45 +50,50 @@ public static OptionDescriptor create(String name,
OptionType optionType,
Class<?> optionValueType,
String help,
Class<?> declaringClass,
Object container,
String fieldName,
OptionKey<?> option) {
return create(name, optionType, optionValueType, help, NO_EXTRA_HELP, declaringClass, fieldName, option, OptionStability.EXPERIMENTAL, false, "");
return create(name, optionType, optionValueType, help, NO_EXTRA_HELP, container, fieldName, option, OptionStability.EXPERIMENTAL, false, "");
}

public static OptionDescriptor create(String name,
OptionType optionType,
Class<?> optionValueType,
String help,
Class<?> declaringClass,
Object container,
String fieldName,
OptionKey<?> option,
OptionStability stability,
boolean deprecated,
String deprecationMessage) {
return create(name, optionType, optionValueType, help, NO_EXTRA_HELP, declaringClass, fieldName, option, stability, deprecated, deprecationMessage);
return create(name, optionType, optionValueType, help, NO_EXTRA_HELP, container, fieldName, option, stability, deprecated, deprecationMessage);
}

public static OptionDescriptor create(String name,
OptionType optionType,
Class<?> optionValueType,
String help,
String[] extraHelp,
Class<?> declaringClass,
Object container,
String fieldName,
OptionKey<?> option,
OptionStability stability,
boolean deprecated,
String deprecationMessage) {
OptionsContainer oc = OptionsContainer.asContainer(container);
Class<?> declaringClass = oc.getDeclaringClass();
assert option != null : declaringClass + "." + fieldName;
OptionDescriptor result = option.getDescriptor();
if (result == null) {
List<String> extraHelpList = extraHelp == null || extraHelp.length == 0 ? Collections.emptyList() : Collections.unmodifiableList(Arrays.asList(extraHelp));
result = new OptionDescriptor(name, optionType, optionValueType, help, extraHelpList, declaringClass, fieldName, option, stability, deprecated, deprecationMessage);
List<String> extraHelpList = extraHelp == null ? List.of() : List.of(extraHelp);
result = new OptionDescriptor(name, optionType, optionValueType, help, extraHelpList, oc, fieldName, option, stability, deprecated, deprecationMessage);
option.setDescriptor(result);
}
assert result.name.equals(name) && result.optionValueType == optionValueType && result.declaringClass == declaringClass && result.fieldName.equals(fieldName) &&
result.optionKey == option : result + " must match with args";
assert result.name.equals(name) : result.name + " != " + name;
assert result.optionValueType == optionValueType : result.optionValueType + " != " + optionValueType;
assert result.getDeclaringClass() == declaringClass : result.getDeclaringClass() + " != " + declaringClass;
assert result.fieldName.equals(fieldName) : result.fieldName + " != " + fieldName;
assert result.optionKey == option : result.optionKey + " != " + option;
return result;
}

Expand All @@ -99,7 +102,7 @@ private OptionDescriptor(String name,
Class<?> optionValueType,
String help,
List<String> extraHelp,
Class<?> declaringClass,
OptionsContainer container,
String fieldName,
OptionKey<?> optionKey,
OptionStability stability,
Expand All @@ -111,7 +114,7 @@ private OptionDescriptor(String name,
this.help = help;
this.extraHelp = extraHelp;
this.optionKey = optionKey;
this.declaringClass = declaringClass;
this.container = container;
this.fieldName = fieldName;
this.stability = stability;
this.deprecated = deprecated || deprecationMessage != null && !deprecationMessage.isEmpty();
Expand All @@ -128,7 +131,7 @@ public Class<?> getOptionValueType() {
}

/**
* Gets a descriptive help message for the option. This message should be self contained without
* Gets a descriptive help message for the option. This message should be self-contained without
* relying on {@link #getExtraHelp() extra help lines}.
*
* @see Option#help()
Expand All @@ -150,7 +153,7 @@ public List<String> getExtraHelp() {
* a user specified value for the option from the environment.
*/
public String getName() {
return name;
return container.prefixed(name);
}

/**
Expand All @@ -167,8 +170,18 @@ public OptionKey<?> getOptionKey() {
return optionKey;
}

/**
* Gets metadata about the class declaring the option.
*/
public OptionsContainer getContainer() {
return container;
}

/**
* Gets the class declaring the option.
*/
public Class<?> getDeclaringClass() {
return declaringClass;
return container.getDeclaringClass();
}

public String getFieldName() {
Expand Down Expand Up @@ -196,16 +209,6 @@ public boolean isDeprecated() {
return deprecated;
}

/**
* Determines if this descriptor is service loaded.
*
* @see OptionGroup#registerAsService()
*/
public boolean isServiceLoaded() {
OptionGroup group = getDeclaringClass().getAnnotation(OptionGroup.class);
return group == null || group.registerAsService();
}

/**
* Returns the deprecation reason and the recommended replacement.
*/
Expand Down
8 changes: 8 additions & 0 deletions compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/options/OptionDescriptors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@
* An interface to a set of {@link OptionDescriptor}s.
*/
public interface OptionDescriptors extends Iterable<OptionDescriptor> {

/**
* Gets metadata about the class in which the options are declared.
*/
default OptionsContainer getContainer() {
return null;
}

/**
* Gets the {@link OptionDescriptor} matching a given option name or {@code null} if this option
* descriptor set doesn't contain a matching option name.
Expand Down
65 changes: 0 additions & 65 deletions compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/options/OptionGroup.java
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/options/OptionKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ protected boolean checkDescriptorExists() {
stackTrace[1].getClassName().equals(OptionKey.class.getName()) &&
stackTrace[1].getMethodName().equals("getValue")) {
String caller = stackTrace[2].getClassName();
result.append(" In suite.py, add GRAAL_OPTIONS_PROCESSOR to the \"annotationProcessors\" attribute of the project containing ");
result.append(" In suite.py, add compiler:GRAAL_PROCESSOR to the \"annotationProcessors\" attribute of the project containing ");
result.append(caller);
}
throw new AssertionError(result.toString());
Expand Down
Loading
Loading