This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new af8b417fc2 GROOVY-12138: default invokedynamic property writes on
(opt-out) + parity guard
af8b417fc2 is described below
commit af8b417fc26a4186411e18ea372e4ec2ebd56259
Author: Paul King <[email protected]>
AuthorDate: Sun Jul 12 06:31:33 2026 +1000
GROOVY-12138: default invokedynamic property writes on (opt-out) + parity
guard
Flip groovy.indy.setproperty from opt-in (default off) to opt-out
(default on); set -Dgroovy.indy.setproperty=false to disable, matching
the getBooleanSafe(name, default) pattern used for cold.reflection.
The full suite (15709 tests) passes with the flag default-on, so no
existing test asserts flag-off-specific property-write bytecode and none
needs a skip guard — the behaviour-preserving adapter fallback means the
suite runs as-is and passes in both states.
Since the default is now on, the off state is no longer exercised by the
normal run, so adds SetPropertyParityTest (in the ColdReflectionParityTest
style): the flag is static-final at IndyCallSiteWriter class-init and
cannot be toggled in-process, so it compiles+runs a property-write
corpus in two JVMs (=true / =false) and asserts byte-identical
transcripts, plus a bytecode check that emission is genuinely gated
(indy setProperty sites when on, none when off) so the parity check
cannot pass vacuously.
---
.../classgen/asm/indy/IndyCallSiteWriter.java | 9 +-
.../groovy/perf/SetPropertyParityTest.groovy | 112 +++++++++++++++++++++
.../groovy/perf/set-property-parity-corpus.groovy | 95 +++++++++++++++++
3 files changed, 212 insertions(+), 4 deletions(-)
diff --git
a/src/main/java/org/codehaus/groovy/classgen/asm/indy/IndyCallSiteWriter.java
b/src/main/java/org/codehaus/groovy/classgen/asm/indy/IndyCallSiteWriter.java
index a2efbff62a..e1f77802cd 100644
---
a/src/main/java/org/codehaus/groovy/classgen/asm/indy/IndyCallSiteWriter.java
+++
b/src/main/java/org/codehaus/groovy/classgen/asm/indy/IndyCallSiteWriter.java
@@ -82,13 +82,14 @@ public class IndyCallSiteWriter extends CallSiteWriter {
}
/**
- * Experimental (GROOVY-12138), guarded by the {@code
groovy.indy.setproperty}
- * compile-time flag: emit an {@code invokedynamic} call site for property
+ * GROOVY-12138: emit an {@code invokedynamic} call site for property
* writes instead of the static {@code ScriptBytecodeAdapter.setProperty}
* call, giving writes the same call-site caching, guarding, and JIT
- * inlining that reads have had via {@code getProperty} sites.
+ * inlining that reads have had via {@code getProperty} sites. Compile-time
+ * flag, on by default; set {@code -Dgroovy.indy.setproperty=false} to
+ * disable (opt-out).
*/
- private static final boolean INDY_SET_PROPERTY =
org.apache.groovy.util.SystemUtil.getBooleanSafe("groovy.indy.setproperty");
+ private static final boolean INDY_SET_PROPERTY =
org.apache.groovy.util.SystemUtil.getBooleanSafe("groovy.indy.setproperty",
true);
/** {@inheritDoc} */
@Override
diff --git
a/subprojects/performance/src/test/groovy/org/apache/groovy/perf/SetPropertyParityTest.groovy
b/subprojects/performance/src/test/groovy/org/apache/groovy/perf/SetPropertyParityTest.groovy
new file mode 100644
index 0000000000..0853a4aab6
--- /dev/null
+++
b/subprojects/performance/src/test/groovy/org/apache/groovy/perf/SetPropertyParityTest.groovy
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://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.apache.groovy.perf
+
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * Standing parity guard for invokedynamic property writes (GROOVY-12138,
+ * {@code groovy.indy.setproperty}, on by default). When enabled, a dynamic
+ * property write compiles to an {@code invokedynamic setProperty} call site;
+ * when disabled it compiles to a static {@code
ScriptBytecodeAdapter.setProperty}
+ * call. The fast path only handles provably sender/metaclass-independent
+ * shapes and falls back to the exact classic adapter for everything else, so
+ * behaviour must be identical either way.
+ * <p>
+ * The flag is a compile-time emission flag read once ({@code static final}) at
+ * {@code IndyCallSiteWriter} class-init, so it cannot be toggled in-process.
+ * This test therefore compiles+runs the companion corpus in two fresh JVMs —
+ * setproperty explicitly enabled and disabled — and asserts the transcripts
+ * are byte-identical. A second test confirms the flag genuinely gates emission
+ * (indy sites when on, adapter calls when off) so the parity check cannot pass
+ * vacuously.
+ */
+class SetPropertyParityTest {
+
+ private static final String CORPUS_RESOURCE =
'set-property-parity-corpus.groovy'
+
+ @Test
+ void setPropertyMatchesAdapterDispatch() {
+ String on = runCorpus(true)
+ String off = runCorpus(false)
+ if (on != off) {
+ def a = off.readLines(), b = on.readLines()
+ def diff = (0..<Math.max(a.size(), b.size())).findResults { i ->
+ def x = i < a.size() ? a[i] : '<missing>'
+ def y = i < b.size() ? b[i] : '<missing>'
+ x != y ? " line ${i + 1}:\n off: $x\n on : $y" : null
+ }.take(10).join('\n')
+ assert false, "indy setproperty transcript diverged from the
adapter path:\n$diff"
+ }
+ assertTrue(on.readLines().size() >= 15, "corpus produced too few
checks (${on.readLines().size()})")
+ }
+
+ @Test
+ void setPropertyGatedByFlag() {
+ // compile a probe with the flag on and off and count invokedynamic
+ // setProperty sites, so the parity test cannot pass vacuously (e.g. if
+ // the flag stopped reaching the compiler).
+ int on = countIndySetProperty(true)
+ int off = countIndySetProperty(false)
+ assertTrue(on >= 1, "expected invokedynamic setProperty site(s) when
enabled, got $on")
+ assertEquals(0, off, "expected no invokedynamic setProperty sites when
disabled, got $off")
+ }
+
+ // --- helpers ---
+
+ private static String runCorpus(boolean setProperty) {
+ def corpus = File.createTempFile('set-property-parity', '.groovy')
+ corpus.deleteOnExit()
+ corpus.bytes =
SetPropertyParityTest.getResourceAsStream(CORPUS_RESOURCE).bytes
+ def out = new StringBuilder()
+ int rc = fork(setProperty, ['groovy.ui.GroovyMain',
corpus.absolutePath], out)
+ assertEquals(0, rc, "corpus JVM (setProperty=$setProperty) failed
rc=$rc:\n$out")
+ return out.toString()
+ }
+
+ private static int countIndySetProperty(boolean setProperty) {
+ def dir = File.createTempDir()
+ dir.deleteOnExit()
+ def probe = new File(dir, 'SetProbe.groovy')
+ probe.text = 'class B { String name; int count }\ndef b = new
B()\nb.name = "x"\nb.count = 1\n'
+ def compileOut = new StringBuilder()
+ int rc = fork(setProperty,
['org.codehaus.groovy.tools.FileSystemCompiler', '-d', dir.absolutePath,
probe.absolutePath], compileOut)
+ assertEquals(0, rc, "probe compile (setProperty=$setProperty)
failed:\n$compileOut")
+ // disassemble and count invokedynamic setProperty sites in the script
class
+ def javap = new File(System.getProperty('java.home'),
'bin/javap').absolutePath
+ def out = new StringBuilder(), err = new StringBuilder()
+ def p = [javap, '-c', '-p', '-classpath', dir.absolutePath,
'SetProbe'].execute()
+ p.waitForProcessOutput(out, err)
+ return out.readLines().count { it.contains('invokedynamic') &&
it.contains('setProperty') }
+ }
+
+ private static int fork(boolean setProperty, List<String> mainAndArgs,
StringBuilder mergedOutput) {
+ def java = new File(System.getProperty('java.home'),
'bin/java').absolutePath
+ def cp = System.getProperty('java.class.path')
+ def cmd = [java, '-cp', cp,
"-Dgroovy.indy.setproperty=${setProperty}".toString()] + mainAndArgs
+ def pb = new ProcessBuilder(cmd)
+ pb.redirectErrorStream(true)
+ def process = pb.start()
+ mergedOutput.append(process.inputStream.text)
+ return process.waitFor()
+ }
+}
diff --git
a/subprojects/performance/src/test/resources/org/apache/groovy/perf/set-property-parity-corpus.groovy
b/subprojects/performance/src/test/resources/org/apache/groovy/perf/set-property-parity-corpus.groovy
new file mode 100644
index 0000000000..4b02d58214
--- /dev/null
+++
b/subprojects/performance/src/test/resources/org/apache/groovy/perf/set-property-parity-corpus.groovy
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://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.
+ */
+
+// Differential parity corpus for groovy.indy.setproperty (GROOVY-12138),
+// driven by SetPropertyParityTest. Dynamic property writes throughout (no
+// @CompileStatic) so each becomes an invokedynamic setProperty site when the
+// flag is on and a ScriptBytecodeAdapter.setProperty call when off. Prints a
+// deterministic transcript; the test compiles+runs it in separate JVMs with
+// the flag on and off and asserts the transcripts are byte-identical (the
+// non-fast-path cases fall back to the exact classic adapter, so behaviour is
+// preserved by construction). Keep all inputs/outputs deterministic when
+// extending it.
+
+class Bean {
+ String name
+ int count
+ long total
+ public double raw // public field, no setter
+ private String hidden = 'h'
+ String getHidden() { hidden }
+ static String shared = 's'
+ Boolean flag
+}
+class Fluent {
+ String label
+ Fluent setLabel(String l) { this.label = 'set:' + l; return this } //
non-void setter
+}
+class Custom {
+ def stored = [:]
+ void setProperty(String name, Object value) { stored[name] =
"custom:$value" }
+}
+
+def out = new StringBuilder()
+Closure rec = { String tag, Closure body ->
+ String line
+ try {
+ line = "$tag | OK | ${body()}"
+ } catch (Throwable t) {
+ line = "$tag | EX | ${t.getClass().name}: ${t.message}"
+ }
+ out.append(line).append('\n')
+}
+
+// setter / field writes, repeated to cross the promotion threshold
+rec('setter.string') { def b = new Bean(); 300.times { i -> b.name = "n$i" };
b.name }
+rec('setter.int') { def b = new Bean(); 300.times { i -> b.count = i };
b.count }
+rec('field.public') { def b = new Bean(); 50.times { i -> b.raw = i + 0.5d };
b.raw }
+// coercion (needs the adapter path): Integer -> long, GString -> String
+rec('coerce.long') { def b = new Bean(); b.total = 5; b.total }
+rec('coerce.gstr') { def b = new Bean(); def who = 'world'; b.name = "hi
$who"; b.name.class.simpleName + ':' + b.name }
+// alternating value types at one call site (re-selection)
+rec('alttypes') { def b = new Bean(); def r = []; ['a', 1, 'b', 2].each {
v -> b.name = "$v"; r << b.name }; r.toString() }
+// custom setProperty interception (must decline the fast path)
+rec('custom.set') { def c = new Custom(); 30.times { c.answer = 42 };
c.stored.answer }
+// static property via Class receiver
+rec('static.prop') { Bean.shared = 'updated'; Bean.shared }
+// metaclass change after caching
+rec('emc') {
+ def b0 = new Bean(); 60.times { b0.name = 'x' }
+ Bean.metaClass.setName = { String v -> delegate.@name = 'MC:' + v }
+ def b1 = new Bean(); b1.name = 'y'
+ def afterEmc = b1.name
+ GroovySystem.metaClassRegistry.removeMetaClass(Bean)
+ def b2 = new Bean(); b2.name = 'z'
+ "$afterEmc|${b2.name}"
+}
+// read-only / missing property errors
+rec('missing') { def b = new Bean(); b.nonexistent = 1; 'no-error' }
+rec('readonly') { def b = new Bean(); b.hidden = 'nope'; 'no-error' }
+// null and Boolean property
+rec('null.bool') { def b = new Bean(); b.flag = true; def a = b.flag; b.flag =
null; "$a|${b.flag}" }
+rec('null.str') { def b = new Bean(); b.name = null; b.name }
+// non-void (fluent) setter
+rec('fluent') { def f = new Fluent(); 25.times { f.label = 'L' }; f.label }
+// maps and expando
+rec('map') { def m = [:]; 40.times { i -> m.k = i }; m.k }
+rec('expando') { def e = new Expando(); 40.times { i -> e.dyn = i }; e.dyn }
+
+print out.toString()
