Bloody hell, a couple beers and several hours later and I can't get this
patch working.
Attached is a partial patch for optimizing the processing of opt and
rest args in Ruby method definitions. The bonuses are that it doesn't
use an ArrayList at all, it totally skips iterative processing of
arguments in a few cases, and should be cheaper when constructing
restargs arrays in all cases.
Except it doesn't work. Or rather, it doesn't work in ant test, but
works for simple cases.
Compare the old logic (too complicated) with the new logic (too
complicated) and see if you can figure out what's wrong. The current
theory is that changing restargs in the eventual args array to be an
actual subarray rather than just "more elements" has broken some JRuby
internals.
I'd love to be able to land this, but I'm not going to get it working
this evening. Give it a go if you love array juggling.
- Charlie
Index: src/org/jruby/internal/runtime/methods/DefaultMethod.java
===================================================================
--- src/org/jruby/internal/runtime/methods/DefaultMethod.java (revision 3577)
+++ src/org/jruby/internal/runtime/methods/DefaultMethod.java (working copy)
@@ -250,71 +250,108 @@
}
private IRubyObject[] prepareOptOrRestArgs(ThreadContext context, Ruby
runtime, IRubyObject[] args, int expectedArgsCount, int restArg, boolean
hasOptArgs) {
- if (restArg == -1 && hasOptArgs) {
+ if (hasOptArgs) {
+ int argsLength = args.length;
int opt = expectedArgsCount + argsNode.getOptArgs().size();
-
- if (opt < args.length) {
- throw runtime.newArgumentError("wrong # of arguments(" +
args.length + " for " + opt + ")");
+ if (restArg == -1) {
+ // no rest arg, make sure args.length == opt
+ if (opt < args.length) {
+ // too many
+ throw runtime.newArgumentError("wrong # of arguments(" +
args.length + " for " + opt + ")");
+ } else {
+ if (args.length < opt) {
+ // fewer explicit args, but opt can fill in
+ IRubyObject[] newArgs = new IRubyObject[opt];
+ System.arraycopy(args, 0, newArgs, 0, args.length);
+ args = newArgs;
+ } else {
+ // exactly as many args as needed, proceed to opt
assignment
+ }
+ }
+ } else {
+ // rest arg, make sure args.length = opt + 1
+ if (args.length < expectedArgsCount) {
+ // too few
+ throw runtime.newArgumentError("wrong # of arguments(" +
args.length + " for " + opt + ")");
+ } if (args.length < opt) {
+ // exactly as many args as explicit, rest = nil
+ IRubyObject[] newArgs = new IRubyObject[opt + 1];
+ System.arraycopy(args, 0, newArgs, 0, args.length);
+ args = newArgs;
+ args[opt] = RubyArray.newArrayNoCopy(runtime,
IRubyObject.NULL_ARRAY);
+ context.getCurrentScope().setValue(restArg, args[opt], 0);
+ } else {
+ // more args than explicit
+ IRubyObject[] newArgs = new IRubyObject[opt + 1];
+ IRubyObject[] restArgs = new IRubyObject[args.length -
opt];
+ System.arraycopy(args, 0, newArgs, 0, opt);
+ System.arraycopy(args, opt, restArgs, 0, args.length -
opt);
+ newArgs[opt] = RubyArray.newArrayNoCopy(runtime, restArgs);
+ args = newArgs;
+ context.getCurrentScope().setValue(restArg, args[opt], 0);
+ }
}
- }
-
- int count = expectedArgsCount;
- if (argsNode.getOptArgs() != null) {
- count += argsNode.getOptArgs().size();
- }
-
- ArrayList allArgs = new ArrayList();
-
- // Combine static and optional args into a single list allArgs
- for (int i = 0; i < count && i < args.length; i++) {
- allArgs.add(args[i]);
- }
-
- if (hasOptArgs) {
- ListNode optArgs = argsNode.getOptArgs();
-
- int j = 0;
- for (int i = expectedArgsCount; i < args.length && j <
optArgs.size(); i++, j++) {
- // in-frame EvalState should already have receiver set as
self, continue to use it
- AssignmentVisitor.assign(runtime, context,
context.getFrameSelf(), optArgs.get(j), args[i], Block.NULL_BLOCK, true);
- expectedArgsCount++;
+
+ // continue on to hanldle opt args, if any
+ int count = expectedArgsCount;
+ if (argsNode.getOptArgs() != null) {
+ count += argsNode.getOptArgs().size();
}
-
- // assign the default values, adding to the end of allArgs
- while (j < optArgs.size()) {
- // in-frame EvalState should already have receiver set as
self, continue to use it
- allArgs.add(EvaluationState.eval(runtime, context,
optArgs.get(j++), context.getFrameSelf(), Block.NULL_BLOCK));
+
+ if (hasOptArgs) {
+ ListNode optArgs = argsNode.getOptArgs();
+
+ int j = 0;
+ int optSize = optArgs.size();
+ for (int i = expectedArgsCount; i < argsLength && j < optSize;
i++, j++) {
+ // in-frame EvalState should already have receiver set as
self, continue to use it
+ AssignmentVisitor.assign(runtime, context,
context.getFrameSelf(), optArgs.get(j), args[i], Block.NULL_BLOCK, true);
+ expectedArgsCount++;
+ }
+
+ // assign the default values, adding to the end of allArgs
+ while (j < optArgs.size()) {
+ // in-frame EvalState should already have receiver set as
self, continue to use it
+ args[expectedArgsCount + j] =
EvaluationState.eval(runtime, context, optArgs.get(j++),
context.getFrameSelf(), Block.NULL_BLOCK);
+ }
}
- }
-
- // build an array from *rest type args, also adding to allArgs
-
- // ENEBO: Does this next comment still need to be done since I killed
hasLocalVars:
- // move this out of the scope.hasLocalVariables() condition to deal
- // with anonymous restargs (* versus *rest)
-
-
- // none present ==> -1
- // named restarg ==> >=0
- // anonymous restarg ==> -2
- if (restArg != -1) {
- for (int i = expectedArgsCount; i < args.length; i++) {
- allArgs.add(args[i]);
- }
-
- // only set in scope if named
- if (restArg >= 0) {
- RubyArray array = runtime.newArray(args.length -
expectedArgsCount);
- for (int i = expectedArgsCount; i < args.length; i++) {
- array.append(args[i]);
+
+ // build an array from *rest type args, also adding to allArgs
+
+ // ENEBO: Does this next comment still need to be done since I
killed hasLocalVars:
+ // move this out of the scope.hasLocalVariables() condition to deal
+ // with anonymous restargs (* versus *rest)
+
+ return args;
+ } else {
+ if (restArg == -1) {
+ throw new RuntimeException("We should not be here if there's
no opt and no rest args. Report this at www.jruby.org.");
+ } else {
+ // rest arg, make sure args.length >= expected + 1
+ if (args.length < expectedArgsCount) {
+ // too few
+ throw runtime.newArgumentError("wrong # of arguments(" +
args.length + " for " + expectedArgsCount + ")");
+ } if (args.length == expectedArgsCount) {
+ // exactly as many args as explicit, rest = nil
+ IRubyObject[] newArgs = new IRubyObject[expectedArgsCount
+ 1];
+ System.arraycopy(args, 0, newArgs, 0, args.length);
+ args = newArgs;
+ args[restArg] = RubyArray.newArrayNoCopy(runtime,
IRubyObject.NULL_ARRAY);
+ context.getCurrentScope().setValue(restArg, args[restArg],
0);
+ return args;
+ } else {
+ // more args than explicit
+ IRubyObject[] newArgs = new IRubyObject[expectedArgsCount
+ 1];
+ IRubyObject[] restArgs = new IRubyObject[args.length -
expectedArgsCount];
+ System.arraycopy(args, 0, newArgs, 0, expectedArgsCount);
+ System.arraycopy(args, expectedArgsCount, restArgs, 0,
args.length - expectedArgsCount);
+ newArgs[expectedArgsCount] =
RubyArray.newArrayNoCopy(runtime, restArgs);
+ args = newArgs;
+ context.getCurrentScope().setValue(restArg,
args[expectedArgsCount], 0);
+ return args;
}
-
- context.getCurrentScope().setValue(restArg, array, 0);
}
}
-
- args = (IRubyObject[])allArgs.toArray(new IRubyObject[allArgs.size()]);
- return args;
}
private void traceReturn(ThreadContext context, Ruby runtime, RubyBinding
binding, String name) {
Index: src/org/jruby/RubyArray.java
===================================================================
--- src/org/jruby/RubyArray.java (revision 3576)
+++ src/org/jruby/RubyArray.java (working copy)
@@ -327,6 +327,17 @@
return new RubyArray(runtime, args);
}
+ public static RubyArray newArrayNoCopy(Ruby runtime, IRubyObject[] args,
int start, int len) {
+ return new RubyArray(runtime, args, start, len);
+ }
+
+ public static RubyArray newArrayNoCopyShared(Ruby runtime, IRubyObject[]
args, int start, int len) {
+ RubyArray result = new RubyArray(runtime, args, start, len);
+ result.shared = true;
+
+ return result;
+ }
+
public static RubyArray newArrayNoCopyLight(Ruby runtime, IRubyObject[]
args) {
RubyArray arr = new RubyArray(runtime, false);
arr.values = args;
@@ -358,6 +369,16 @@
values = vals;
realLength = vals.length;
}
+
+ /*
+ * plain internal array assignment
+ */
+ private RubyArray(Ruby runtime, IRubyObject[] vals, int start, int len) {
+ super(runtime, runtime.getArray());
+ values = vals;
+ begin = start;
+ realLength = len;
+ }
/* rb_ary_new2
* just allocates the internal array
---------------------------------------------------------------------
To unsubscribe from this list please visit:
http://xircles.codehaus.org/manage_email
