Make dynamic "once" regexp update atomic.

See #2798 for discussion about whether the entire body of a dynamic "once" regexp should be atomic.
jruby · Apr 2, 2015 · 7a750e2 · 7a750e2
1 parent edd35d8
commit 7a750e2
Showing 2 changed files with 56 additions and 26 deletions.
diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildDynRegExpInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildDynRegExpInstr.java
@@ -15,13 +15,18 @@
 import org.jruby.runtime.builtin.IRubyObject;
 import org.jruby.util.RegexpOptions;
 
+import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
+
 // Represents a dynamic regexp in Ruby
 // Ex: /#{a}#{b}/
 public class BuildDynRegExpInstr extends ResultBaseInstr {
     final private RegexpOptions options;
 
     // Cached regexp
-    private RubyRegexp rubyRegexp;
+    private volatile RubyRegexp rubyRegexp;
+
+    private static final AtomicReferenceFieldUpdater<BuildDynRegExpInstr, RubyRegexp> UPDATER =
+            AtomicReferenceFieldUpdater.newUpdater(BuildDynRegExpInstr.class, RubyRegexp.class, "rubyRegexp");
 
     public BuildDynRegExpInstr(Variable result, Operand[] pieces, RegexpOptions options) {
         super(Operation.BUILD_DREGEXP, result, pieces);
@@ -81,7 +86,13 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
             RubyString   pattern = RubyRegexp.preprocessDRegexp(context.runtime, pieces, options);
             RubyRegexp re = RubyRegexp.newDRegexp(context.runtime, pattern, options);
             re.setLiteral();
-            rubyRegexp = re;
+            if (options.isOnce()) {
+                // Atomically update this, so we only see one instance cached ever.
+                // See MRI's ruby/test_regexp.rb, test_once_multithread
+                UPDATER.compareAndSet(this, null, re);
+            } else {
+                rubyRegexp = re;
+            }
         }
 
         return rubyRegexp;

diff --git a/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java b/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java
@@ -45,6 +45,7 @@
 import org.jruby.util.RegexpOptions;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
 
 import static org.jruby.util.CodegenUtils.ci;
 import static org.jruby.util.CodegenUtils.p;
@@ -177,11 +178,11 @@ private static String keyFor(Object... objs) {
         return sb.toString();
     }
 
-    public void pushDRegexp(Runnable callback, RegexpOptions options, int arity) {
+    public void pushDRegexp(final Runnable callback, final RegexpOptions options, final int arity) {
         if (arity > MAX_ARGUMENTS) throw new NotCompilableException("dynamic regexp has more than " + MAX_ARGUMENTS + " elements");
 
         SkinnyMethodAdapter adapter2;
-        String incomingSig = sig(RubyRegexp.class, params(ThreadContext.class, RubyString.class, arity, int.class));
+        final String incomingSig = sig(RubyRegexp.class, params(ThreadContext.class, RubyString.class, arity, int.class));
 
         if (!getClassData().dregexpMethodsDefined.contains(arity)) {
             adapter2 = new SkinnyMethodAdapter(
@@ -203,29 +204,47 @@ public void pushDRegexp(Runnable callback, RegexpOptions options, int arity) {
             getClassData().dregexpMethodsDefined.add(arity);
         }
 
-        String cacheField = null;
-        Label done = null;
-
         if (options.isOnce()) {
-            // need to cache result forever
-            cacheField = "dregexp" + getClassData().callSiteCount.getAndIncrement();
-            done = new Label();
-            adapter.getClassVisitor().visitField(Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC, cacheField, ci(RubyRegexp.class), null, null).visitEnd();
-            adapter.getstatic(getClassData().clsName, cacheField, ci(RubyRegexp.class));
-            adapter.dup();
-            adapter.ifnonnull(done);
-            adapter.pop();
-        }
-
-        // call synthetic method if we still need to build dregexp
-        callback.run();
-        adapter.ldc(options.toEmbeddedOptions());
-        adapter.invokestatic(getClassData().clsName, "dregexp:" + arity, incomingSig);
-
-        if (done != null) {
-            adapter.dup();
-            adapter.putstatic(getClassData().clsName, cacheField, ci(RubyRegexp.class));
-            adapter.label(done);
+            // need to cache result forever, but do it under sync to avoid double init
+            final String cacheField = "dregexp" + getClassData().callSiteCount.getAndIncrement();
+            final Label done = new Label();
+            final String clsDesc = "L" + getClassData().clsName.replaceAll("\\.", "/") + ";";
+            adapter.ldc(Type.getType(clsDesc));
+            adapter.monitorenter();
+            adapter.trycatch(p(Throwable.class),
+                    new Runnable() {
+                        public void run() {
+                            adapter.getClassVisitor().visitField(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, cacheField, ci(RubyRegexp.class), null, null).visitEnd();
+                            adapter.getstatic(getClassData().clsName, cacheField, ci(RubyRegexp.class));
+                            adapter.dup();
+                            adapter.ifnonnull(done);
+                            adapter.pop();
+
+                            // call synthetic method if we still need to build dregexp
+                            callback.run();
+                            adapter.ldc(options.toEmbeddedOptions());
+                            adapter.invokestatic(getClassData().clsName, "dregexp:" + arity, incomingSig);
+
+                            adapter.dup();
+                            adapter.putstatic(getClassData().clsName, cacheField, ci(RubyRegexp.class));
+                            adapter.label(done);
+
+                            adapter.ldc(Type.getType(clsDesc));
+                            adapter.monitorexit();
+                        }
+                    },
+                    new Runnable() {
+                        public void run() {
+                            adapter.ldc(Type.getType(clsDesc));
+                            adapter.monitorexit();
+                            adapter.athrow();
+                        }
+                    });
+        } else {
+            // call synthetic method if we still need to build dregexp
+            callback.run();
+            adapter.ldc(options.toEmbeddedOptions());
+            adapter.invokestatic(getClassData().clsName, "dregexp:" + arity, incomingSig);
         }
     }