Re: RFA: Changes to interpreter to avoid non-debugging slowdown

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:

Kyle> This patch creates a second version of _Jv_InterpMethod::run() using 
Kyle> preprocessor directives to allow the debugging info to be collected only 
Kyle> when the interpreter is passed the debug command line argument.  This 
Kyle> avoids slowdowns in non-debugging cases by not gathering debug 
Kyle> information or sending events when not debugging.

When posting put a bit more rationale in for a big change like this.

We talked about it offline and the idea is, we need to track the types
of stack and local variable slots while interpreting if the debugging
code is enabled.  However, we don't want to do this unconditionally as
it adds overhead to interpretation.

One idea was to keep the verifier-generated information around, but
this looked costly due to increased memory use and also because we'd
still have to do some work in the interpreter (the verifier only keeps
type states at branch targets).

So, having a second debug-only interpreter seemed more suitable.

This is looking pretty good.  Some problems below.

Kyle> +  //switch debug mode on or off
Kyle> +  static void setDebug(bool debug)
Kyle> +  {
Kyle> +  	run = (debug) ? run_debug : run_no_debug;
Kyle> +  }

Do we really need this as a method on _Jv_InterpMethod?
I was assuming that "debug mode" would be a global choice.
But... you tell me, I don't know how this part is supposed to work.

Kyle> Index: /notnfs/kgallowa/vanilla/libjava/interpret-run.cc
Kyle> ===================================================================
Kyle> --- /notnfs/kgallowa/vanilla/libjava/interpret-run.cc	(revision 0)
Kyle> +++ /notnfs/kgallowa/vanilla/libjava/interpret-run.cc	(revision 0)
Kyle> @@ -0,0 +1,2500 @@
Kyle> +  using namespace java::lang::reflect;

This file needs a copyright header.
Also there ought to be a comment after the copyright header explaining
that this file shouldn't be compiled but instead is only included by
interpret.cc.

Kyle> Index: /notnfs/kgallowa/vanilla/libjava/interpret.cc
Kyle> +#define STOREA(I) 		\
Kyle> +DEBUG_LOCALS_INSN(I,o)	\

It is our style to put a space after the ',' here.

Kyle> +void (*_Jv_InterpMethod::run) (void *, ffi_raw *, _Jv_InterpMethod *) = NULL;

If debug mode is a global then I don't think we need this.  We can
simply have multiple run_normal, etc, methods, and choose the ones we
like when preparing the ffi closures.  That lets us avoid putting an
extra indirect dispatch into every call to an interpreted method.

Kyle> +/* Used to keep track of local variable type
Kyle> + * 
Kyle> + * Possible Types:
Kyle> + * o object
Kyle> + * i integer
Kyle> + * f float
Kyle> + * l long (one slot)
Kyle> + * m long (two slots)
Kyle> + * d double (one slot)
Kyle> + * e double (two slots)
Kyle> + */

I think we don't really need the 'm' and 'e' types -- on a given
platform, we will always either do one or the other.  This knowledge
might as well be available to other parts of libgcj.

Kyle> +#define DEBUG
Kyle> +#undef DEBUG_LOCALS_INSN
Kyle> +#define DEBUG_LOCALS_INSN(s,t) { localsType[s] = 't'; }

This won't do what you think it does.  You'll always see a literal 't'
in the result.

Instead you either want to just pass in character constants (best, if
possible) or do something like:

  localsType[s] = (# t)[0];

Tom