[Truffle] New system for options. #4335

chrisseaton · 2016-11-25T18:47:37Z

We need to disconnect from the JRuby system because:

We don't want dependencies on the core JRuby code as it causes AOT problems.
PolyglotEngine has a configuration system and we should be going through that.
Options need to be compilation final.
Adding options in multiple places is a chore.
Different options should be able to be set for PolyglotEngines in the same VM.

I'm suggesting the system implemented below, with the extra phase that we will code generate some of the classes from the text file. This would be like the parser - we'd commit the generated code as it rarely changes and is small. The code generator would just be a Ruby template - you'd jt build options.

I haven't written the code generator yet. The name NewOptions would just become Options. I didn't make that change to keep this diff small.

All configuration which used to come from RubyInstanceConfig will now be an option. An example of where this is different to before is that arguments (ARGV) would be an option. At the moment there's no way to set this in PolyglotEngine - this new system makes it possible to set it both as a PolyglotEngine configuration option and even in a system property.

I'm proposing we use this one system for all options.

For the difference it makes in the key areas, see

https://github.com/jruby/jruby/compare/truffle-head...truffle-options?expand=1#diff-7251a9f169e965e77978f3841e2c9bfaR118
https://github.com/jruby/jruby/compare/truffle-head...truffle-options?expand=1#diff-56a6b80386767522713adc1aafcd9d45R39 (all fields we use in RubyInstanceConfig would instead become one of these lines)

Opinions and votes for and against please @pitr-ch @bjfish @eregon @nirvdrum.

Don't merge.

chrisseaton · 2016-11-25T18:51:22Z

@jtulach is this what we're supposed to be doing with the config in PolyglotEngine? Making that the place where all options can be set?

eregon · 2016-11-25T21:14:18Z

truffle/src/main/java/org/jruby/truffle/options/NewOptions.java

+
+public class NewOptions {
+
+    public final String[] ARGUMENTS;


Ideally also @CompilationFinal(dimensions = 1)

Ah I didn't know about that - good idea.

eregon · 2016-11-25T21:17:04Z

truffle/src/main/java/org/jruby/truffle/RubyEngine.java

@@ -35,6 +36,7 @@ public RubyEngine(RubyInstanceConfig instanceConfig) {

        engine = PolyglotEngine.newBuilder()
                .config(RubyLanguage.MIME_TYPE, INSTANCE_CONFIG_KEY, instanceConfig)
+                .config(RubyLanguage.MIME_TYPE, OptionsCatalogue.ARGUMENTS.getName(), instanceConfig.getArgv())


Is this line needed?

We're getting rid of the RubyInstanceConfig on the line above when I convert all options over. So this is the only way we'll have to communicate arguments across. And it's a good thing because you'll then be able to set Ruby arguments for code that you invoke from other languages.

How about passing them as a HashMap or so instead?
This could become quickly a long list, isn't it?

Ah, but options can just not be passed and then use the defaults I guess, except from some key things like ARGV coming from non-system property or OptionDescriptor defaults.

It's already a hash map - one that the polyglot engine maintains - these config calls are like put. And it's already private to our language.

There are only ten or so fields like this that we need to pass. We don't need to do this for options that RubyInstanceConfig doesn't currently set.

eregon · 2016-11-25T21:21:24Z

Looks good to me 😃 👍

pitr-ch · 2016-11-28T13:26:00Z

Looks good 👍 Only one suggestion, could the txt file be something structured? E.g.: a Ruby hash in a file using eval File.read('options.rb'), or a Yaml file (, or JSON)?

chrisseaton · 2016-11-28T13:27:20Z

I thought the simple structure was a bonus? I'll write it as Ruby DSL or a Yaml or JSON file if people want. Chime in if you want that changed.

pitr-ch · 2016-11-28T14:02:03Z

But in unstructured txt you have to format manually and then parsed. Hash can be eval-ed (or yaml loaded) and it's done.

nirvdrum · 2016-11-28T21:01:35Z

I agree with Petr. If the idea is the text file isn't going to change that frequently, I'd rather use an established file format, even if it's more verbose.

eregon · 2016-11-28T21:15:21Z

👍 for Yaml.
Alternatively the current format could be easily be made valid CSV, but that's a weird format to manually enter

nirvdrum

I like the look of this overall. I think we still need an argv processor of some sort though. RubyInstanceConfig takes care of a lot internally that isn't represented here.

nirvdrum · 2016-11-28T21:02:27Z

truffle/src/main/java/org/jruby/truffle/options/OptionsCatalogue.java

+
+// This file would be automatically generated from the list of options in the text file.
+
+public class OptionsCatalogue {


I think we settled on using the improved American spelling.

nirvdrum · 2016-11-28T21:10:15Z

core/src/main/java/org/jruby/util/cli/Options.java

@@ -296,7 +296,7 @@

    public static final Option<Integer> TRUFFLE_INSTRUMENTATION_SERVER_PORT = integer(TRUFFLE, "truffle.instrumentation_server_port", 0, "Port number to run an HTTP server on that provides instrumentation services");
    public static final Option<Boolean> TRUFFLE_EXCEPTIONS_STORE_JAVA = bool(TRUFFLE, "truffle.exceptions.store_java", false, "Store the Java exception with the Ruby backtrace");
-    public static final Option<Boolean> TRUFFLE_EXCEPTIONS_PRINT_JAVA = bool(TRUFFLE, "truffle.exceptions.print_java", false, "Print Java exceptions at the point of translating them to Ruby exceptions.");
+    //public static final Option<Boolean> TRUFFLE_EXCEPTIONS_PRINT_JAVA = bool(TRUFFLE, "truffle.exceptions.print_java", false, "Print Java exceptions at the point of translating them to Ruby exceptions.");


I take it this is being removed?

nirvdrum · 2016-11-28T21:10:38Z

truffle/src/main/java/org/jruby/truffle/Options.java

@@ -33,7 +33,7 @@
 import static org.jruby.util.cli.Options.TRUFFLE_ENCODING_COMPATIBLE_QUERY_CACHE;
 import static org.jruby.util.cli.Options.TRUFFLE_ENCODING_LOADED_CLASSES_CACHE;
 import static org.jruby.util.cli.Options.TRUFFLE_EVAL_CACHE;
-import static org.jruby.util.cli.Options.TRUFFLE_EXCEPTIONS_PRINT_JAVA;
+//import static org.jruby.util.cli.Options.TRUFFLE_EXCEPTIONS_PRINT_JAVA;


I take it this is being removed?

nirvdrum · 2016-11-28T21:10:44Z

truffle/src/main/java/org/jruby/truffle/Options.java

@@ -159,7 +159,7 @@

    public final int INSTRUMENTATION_SERVER_PORT = TRUFFLE_INSTRUMENTATION_SERVER_PORT.load();
    public final boolean EXCEPTIONS_STORE_JAVA = TRUFFLE_EXCEPTIONS_STORE_JAVA.load();
-    public final boolean EXCEPTIONS_PRINT_JAVA = TRUFFLE_EXCEPTIONS_PRINT_JAVA.load();
+    //public final boolean EXCEPTIONS_PRINT_JAVA = TRUFFLE_EXCEPTIONS_PRINT_JAVA.load();


I take it this is being removed?

nirvdrum · 2016-11-28T21:24:12Z

truffle/src/main/java/org/jruby/truffle/options/OptionsBuilder.java

+
+    public void set(Properties properties) {
+        for (Map.Entry<Object, Object> property : properties.entrySet()) {
+            final String name = (String) property.getKey();


Is there a case you're trying to handle here by not just calling toString?

Properties are just a very old API and don't use generics.

I meant is there a reason to avoid property.getKey().toString()?

nirvdrum · 2016-11-28T21:27:27Z

truffle/src/main/java/org/jruby/truffle/options/OptionsBuilder.java

+        if (description == null) {
+            //throw new UnsupportedOperationException(name);
+
+            // Don't throw for now - not all the options are transalted across


It might be beneficial to print out a warning (configurable via option!) for any option without a description.

I will do when they're all in this system.

nirvdrum · 2016-11-28T21:31:41Z

truffle/src/main/java/org/jruby/truffle/options/StringArrayOptionDescription.java

+
+    // Allows input such as [foo, "bar", 'baz']. Doesn't support escape sequences.
+
+    private String[] parseStringArray(String string) {


Unless I've misunderstood the intent of this method, I think it could go away if we just used something like JSON or YAML for the config file format.

I guess that wouldn't help the argv case, so maybe a value parser is still necessary. It just seems a bit complex.

Indeed, what is this for? Do we have a use-case to pass a String[] as a String? A comma-separated list is much nicer to parse and to give on the command line: someOption=foo,bar,baz.

I switched to a simple comma separated format, with escaping for commas if needed. The reason for the string format is so you can set things like Ruby ARGV using system properties. Imagine starting a JS node application and for some reason wanting Ruby code called using interop still have an ARGV.

nirvdrum · 2016-11-28T21:34:21Z

truffle/src/main/java/org/jruby/truffle/options/OptionTypeException.java

+ */
+package org.jruby.truffle.options;
+
+public class OptionTypeException extends UnsupportedOperationException {


I think it'd be helpful for debugging if this exception type took the option name as its argument. Or perhaps we'd just enforce a message passed with the option name.

nirvdrum · 2016-11-28T21:44:34Z

truffle/src/main/java/org/jruby/truffle/options/UnknownOptionException.java

+ */
+package org.jruby.truffle.options;
+
+public class UnknownOptionException extends UnsupportedOperationException {


As with OptionTypeException, I think it'd help debugging a lot of if the option name were required.

chrisseaton · 2016-11-30T23:37:04Z

Can people review the generator.

I actually have a problem with my ERB templates giving me extra blank lines. Does anyone know how to fix that?

Other than that, raise any final objections now before I do all the work to switch the current options over and merge.

nirvdrum · 2016-12-01T02:51:08Z

truffle/src/main/java/org/jruby/truffle/options/OptionsBuilder.java

+
+    private final Map<OptionDescription, Object> options = new HashMap<>();
+
+    public void set(Properties properties) {


I can't readily tell, but I suspect we're going to want a boundary on this.

I don't think so - you can't set options after the engine is created, so it can't be fast path.

nirvdrum · 2016-12-01T02:51:59Z

truffle/src/main/java/org/jruby/truffle/options/StringArrayOptionDescription.java

+
+    // Allows input such as foo,bar,baz. You can escape commas.
+
+    private String[] parseStringArray(String string) {


I think we're going to want a boundary on this.

I don't think so - you can't set options after the engine is created, so it can't be fast path.

pitr-ch · 2016-12-01T10:42:54Z

tool/truffle/generate-options.rb

+    }
+
+}
+}).result)


File.write('truffle/src/main/java/org/jruby/truffle/options/NewOptions.java', ERB.new(<<-JAVA).result) /* * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. This * code is released under a tri EPL/GPL/LGPL license. You can use it, * redistribute it and/or modify it under the terms of the: * * Eclipse Public License version 1.0 * GNU General Public License version 2 * GNU Lesser General Public License version 2.1 */ package org.jruby.truffle.options; // This file is automatically generated by options.yml with 'jt build options' import com.oracle.truffle.api.CompilerDirectives.CompilationFinal; import javax.annotation.Generated; @Generated("tool/truffle/generate-options.rb") public class NewOptions { <% options.each do |o| %> <% if o.type.end_with?('[]') %>@CompilationFinal(dimensions=1) <% end %>public final <%= o.type %> <%= o.constant %>; <% end %> NewOptions(OptionsBuilder builder) { <% options.each do |o| %> <%= o.constant %> = builder.getOrDefault(OptionsCatalog.<%= o.constant %>); <% end %> } } JAVA

gives highlighting in idea

Wow! Thanks! There's an example great feature, @eregon.

pitr-ch · 2016-12-01T10:48:29Z

[13] pry(main)> ERB.new("<%= :a -%>\n", nil, '-').result
=> "a"

should help

eregon · 2016-12-01T17:54:29Z

tool/truffle/generate-options.rb

+require 'yaml'
+require 'erb'
+
+options = YAML.load_file('truffle/src/main/java/org/jruby/truffle/options/options.yml')


Should this yml file really be in the middle of Java files?
I think it makes more sense outside, like in tool/truffle.

eregon · 2016-12-01T17:57:26Z

truffle/src/main/java/org/jruby/truffle/options/OptionTypeException.java

+    private final String name;
+
+    public OptionTypeException(String name) {
+        this.name = name;


This should super() and given a meaningful message.

…ons.

eregon · 2016-12-03T12:41:05Z

The full PR looks great, thanks!

chrisseaton added the truffle label Nov 25, 2016

eregon reviewed Nov 25, 2016

View reviewed changes

nirvdrum suggested changes Nov 28, 2016

View reviewed changes

chrisseaton force-pushed the truffle-options branch from ed6e12e to 06242f4 Compare November 30, 2016 21:41

nirvdrum reviewed Dec 1, 2016

View reviewed changes

pitr-ch reviewed Dec 1, 2016

View reviewed changes

eregon reviewed Dec 1, 2016

View reviewed changes

nirvdrum approved these changes Dec 2, 2016

View reviewed changes

eregon approved these changes Dec 2, 2016

View reviewed changes

chrisseaton added 9 commits December 2, 2016 16:06

[Truffle] New system for options.

fdb8822

[Truffle] CompilationFinal dimensions on array options.

cd3470a

[Truffle] American English.

436ca53

[Truffle] Give the option exceptions the name.

104a693

[Truffle] Switch to a simple comma,separated,syntax for String[] opti…

72d5993

…ons.

[Truffle] Switch options configuration to YAML.

0e6037b

[Truffle] Implement the options generator.

63e58cb

[Truffle] Tidy up options generator.

e536c63

[Truffle] Messages for option exceptions.

fe0b953

[Truffle] Move options out of Java source code directory.

5d6b60f

chrisseaton force-pushed the truffle-options branch from f5087f2 to 5d6b60f Compare December 2, 2016 16:06

chrisseaton added 7 commits December 2, 2016 16:15

[Truffle] Broken refactoring.

ee0b534

[Truffle] Support for integer options.

dc380c4

[Truffle] Add support for string options.

2432ff4

[Truffle] Support reference default option values.

9da1532

[Truffle] Switch to new options system.

4f96f35

[Truffle] Fix test option.

2b0b653

[Truffle] Use some static options for now.

352db68

chrisseaton force-pushed the truffle-options branch from 2e2d741 to 352db68 Compare December 3, 2016 00:12

chrisseaton merged commit 9e83c7f into truffle-head Dec 3, 2016

chrisseaton deleted the truffle-options branch December 3, 2016 10:38

enebo modified the milestone: truffle-dev Jan 10, 2017

enebo added this to the Non-Release milestone Dec 7, 2017


		// This file would be automatically generated from the list of options in the text file.

		public class OptionsCatalogue {


		// Allows input such as [foo, "bar", 'baz']. Doesn't support escape sequences.

		private String[] parseStringArray(String string) {


		private final Map<OptionDescription, Object> options = new HashMap<>();

		public void set(Properties properties) {


		// Allows input such as foo,bar,baz. You can escape commas.

		private String[] parseStringArray(String string) {

[Truffle] New system for options. #4335

[Truffle] New system for options. #4335

Conversation

chrisseaton commented Nov 25, 2016 • edited

chrisseaton commented Nov 25, 2016

eregon Nov 25, 2016 • edited

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

eregon commented Nov 25, 2016

pitr-ch commented Nov 28, 2016

chrisseaton commented Nov 28, 2016

pitr-ch commented Nov 28, 2016

nirvdrum commented Nov 28, 2016

eregon commented Nov 28, 2016

nirvdrum 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

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

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

chrisseaton commented Nov 30, 2016

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

pitr-ch commented Dec 1, 2016

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

eregon commented Dec 3, 2016

chrisseaton commented Nov 25, 2016 •

edited

eregon Nov 25, 2016 •

edited