Skip to content

Commit

Permalink
Partially undo kwarg optz from jruby#8095
Browse files Browse the repository at this point in the history 
In jruby#8095 (jruby/jruby@121c60fb) I optimized the JIT to
statically compile in the logic for ruby2_keywords at the time of
JIT. Unfortunately there turned out to be too many cases where
this flag can change at runtime, leading to bugs like jruby#8119
where a previously-jitted scope gets flipped to ruby2_keywords
later on.

This patch partially reverts that optimization and resumes passing
in the live ruby2_keywords value from the scope. We can revisit
this in the future when it is possible to throw out and re-compile
scopes that get this flag set later on.

Fixes jruby#8119
  • Loading branch information
@headius
headius committed Apr 23, 2024
1 parent 5649433 commit bf9508f
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 70 deletions.
49 changes: 7 additions & 42 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,14 +670,18 @@ public static IRubyObject undefined() {
// specific arity methods in JIT will only be fixed arity meaning no kwargs and no rest args.
// this logic is the same as recieveKeywords but we know it will never be a keyword argument (jit
// will save %undefined as the keyword value).
@JIT // Only used for specificArity JITted methods with at least one parameter
public static IRubyObject receiveSpecificArityKeywords(ThreadContext context, IRubyObject last) {
// Due to jruby/jruby#8119 and the potential for ruby2_keywords flags to change after JIT, jitted code will always
// call on this path and pass in the live value of ruby2_keywords from the scope.
@JIT
public static IRubyObject receiveSpecificArityKeywords(ThreadContext context, IRubyObject last, boolean ruby2Keywords) {
if (!(last instanceof RubyHash)) {
ThreadContext.clearCallInfo(context);
return last;
}

return receiveSpecificArityHashKeywords(context, last);
return ruby2Keywords ?
receiveSpecificArityRuby2HashKeywords(context, last) :
receiveSpecificArityHashKeywords(context, last);
}

private static IRubyObject receiveSpecificArityHashKeywords(ThreadContext context, IRubyObject last) {
Expand All @@ -687,17 +691,6 @@ private static IRubyObject receiveSpecificArityHashKeywords(ThreadContext contex
return receiverSpecificArityKwargsCommon(context, last, callInfo, isKwarg);
}

// same as receiveSpecificArityKeywords but only used when the scope demands ruby2 keywords.
@JIT // Only used for specificArity JITted methods with at least one parameter
public static IRubyObject receiveSpecificArityRuby2Keywords(ThreadContext context, IRubyObject last) {
if (!(last instanceof RubyHash)) {
ThreadContext.clearCallInfo(context);
return last;
}

return receiveSpecificArityRuby2HashKeywords(context, last);
}

private static IRubyObject receiveSpecificArityRuby2HashKeywords(ThreadContext context, IRubyObject last) {
int callInfo = ThreadContext.resetCallInfo(context);
boolean isKwarg = (callInfo & CALL_KEYWORD) != 0;
Expand Down Expand Up @@ -759,34 +752,6 @@ public static IRubyObject receiveNormalKeywordsNoRestNoKeywords(ThreadContext co
return UNDEFINED;
}

/**
* Simplified receiveKeywords when receiver IS NOT a ruby2_keywords method.
*
* @param context
* @param args
* @param hasRestArgs
* @param acceptKeywords
* @return the prepared kwargs hash, or UNDEFINED as a sigil for no kwargs
*/
@JIT
public static IRubyObject receiveNormalKeywords(ThreadContext context, IRubyObject[] args, boolean hasRestArgs, boolean acceptKeywords) {
return receiveKeywords(context, args, hasRestArgs, acceptKeywords, false);
}

/**
* Simplified receiveKeywords when receiver IS a ruby2_keywords method.
*
* @param context
* @param args
* @param hasRestArgs
* @param acceptKeywords
* @return the prepared kwargs hash, or UNDEFINED as a sigil for no kwargs
*/
@JIT
public static IRubyObject receiveRuby2Keywords(ThreadContext context, IRubyObject[] args, boolean hasRestArgs, boolean acceptKeywords) {
return receiveKeywords(context, args, hasRestArgs, acceptKeywords, true);
}

/**
* Handle incoming keyword arguments given the receiver's rest arg, keyword acceptance, and need for ruby2_keywords.
*
Expand Down
39 changes: 11 additions & 28 deletions core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1267,20 +1267,6 @@ public void CallInstr(CallInstr callInstr) {
}

private void compileCallCommon(IRBytecodeAdapter m, CallBase call) {
// certain methods interfere with compilation and are rejected
switch (call.getName().idString()) {
case "ruby2_keywords":
/*
if called against a child scope (method or block) from a parent that has been compiled, it will have
no effect, since the child is already compiled to use non-ruby2_keywords logic. We reject compiling
such scopes, to avoid too-eagerly compiling a child that is intended to change to ruby2_keywords
logic later. The use of ruby2_keywords is already uncommon but using it in a scope that also is
called frequently enough to JIT is even more uncommon (at time of writing, this pattern only appears
in tests for ruby2_keywords behavior, in mri/ruby/test_keyword.rb).
*/
throw new NotCompilableException("ruby2_keywords can change behavior of already-compiled code");
}

boolean functional = call.getCallType() == CallType.FUNCTIONAL || call.getCallType() == CallType.VARIABLE;

Operand[] args = call.getCallArgs();
Expand Down Expand Up @@ -1962,7 +1948,8 @@ public void PrepareBlockArgsInstr(PrepareBlockArgsInstr instr) {
jvmMethod().loadSelfBlock();
jvmMethod().loadArgs();
jvmAdapter().ldc(jvm.methodData().scope.receivesKeywordArgs());
jvmAdapter().ldc(jvm.methodData().scope.isRuby2Keywords());
jvmMethod().loadStaticScope();
jvmAdapter().invokevirtual(p(StaticScope.class), "isRuby2Keywords", sig(boolean.class));
jvmMethod().invokeIRHelper("prepareBlockArgs", sig(IRubyObject[].class, ThreadContext.class, Block.class, IRubyObject[].class, boolean.class, boolean.class));
jvmMethod().storeArgs();
}
Expand Down Expand Up @@ -2156,15 +2143,15 @@ public void ReceiveJRubyExceptionInstr(ReceiveJRubyExceptionInstr receiveexcepti
@Override
public void ReceiveKeywordsInstr(ReceiveKeywordsInstr instr) {
int argsLength = jvm.methodData().specificArity;
boolean ruby2KeywordsMethod = jvm.methodData().scope.isRuby2Keywords();

if (argsLength >= 0) {
if (argsLength > 0) {
jvmMethod().loadContext();
jvmAdapter().aload(3 + argsLength - 1); // 3 - 0-2 are not args // FIXME: This should get abstracted
jvmMethod().invokeIRHelper(
ruby2KeywordsMethod ? "receiveSpecificArityRuby2Keywords" : "receiveSpecificArityKeywords",
sig(IRubyObject.class, ThreadContext.class, IRubyObject.class));
jvmMethod().loadStaticScope();
jvmAdapter().invokevirtual(p(StaticScope.class), "isRuby2Keywords", sig(boolean.class));
jvmMethod().invokeIRHelper("receiveSpecificArityKeywords",
sig(IRubyObject.class, ThreadContext.class, IRubyObject.class, boolean.class));
jvmAdapter().astore(3 + argsLength - 1); // 3 - 0-2 are not args // FIXME: This should get abstracted
} else {
jvmMethod().loadContext();
Expand All @@ -2174,15 +2161,11 @@ public void ReceiveKeywordsInstr(ReceiveKeywordsInstr instr) {
} else {
jvmMethod().loadContext();
jvmMethod().loadArgs();
if (!(ruby2KeywordsMethod || instr.hasRestArg() || instr.acceptsKeywords())) {
jvmMethod().invokeIRHelper("receiveNormalKeywordsNoRestNoKeywords", sig(IRubyObject.class, ThreadContext.class, IRubyObject[].class));
} else {
jvmAdapter().ldc(instr.hasRestArg());
jvmAdapter().ldc(instr.acceptsKeywords());
jvmMethod().invokeIRHelper(
ruby2KeywordsMethod ? "receiveRuby2Keywords" : "receiveNormalKeywords",
sig(IRubyObject.class, ThreadContext.class, IRubyObject[].class, boolean.class, boolean.class));
}
jvmAdapter().ldc(instr.hasRestArg());
jvmAdapter().ldc(instr.acceptsKeywords());
jvmMethod().loadStaticScope();
jvmAdapter().invokevirtual(p(StaticScope.class), "isRuby2Keywords", sig(boolean.class));
jvmMethod().invokeIRHelper("receiveKeywords", sig(IRubyObject.class, ThreadContext.class, IRubyObject[].class, boolean.class, boolean.class, boolean.class));
}
jvmStoreLocal(instr.getResult());
}
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/org/jruby/parser/StaticScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,4 +731,8 @@ public Collection<String> getInstanceVariableNames() {
public void setInstanceVariableNames(Collection<String> ivarWrites) {
this.ivarNames = ivarWrites;
}

public boolean isRuby2Keywords() {
return irScope.isRuby2Keywords();
}
}

0 comments on commit bf9508f

Please sign in to comment.