PATCH: interpreter.cc bug patch and performance improvements
Tom Tromey
tromey@redhat.com
Tue Jan 8 11:07:00 GMT 2002
>>>>> "Chris" == Chris Sears <cbsears_sf@yahoo.com> writes:
I read through your patch.
Chris> The bug is fairly simple, no NULLCHECK for insn_arraylength.
I agree, this is definitely a bug.
Chris> The performance improvements fall into two categories:
Chris> 1) in array loads and stores, pushing the SAVE_PC into
Chris> the NULLCHECK throw block.
I think we can't do this, because on many platforms NULLCHECK is
simply defined away -- we rely on the segv handler to generate the
NullPointerException. In this situation we still have to have the
saved PC correct.
Do you know whether this change in isolation really does help
performance? And if so, how much?
One idea might be for us to have NULLCHECK expand to SAVE_PC in some
situations. We'd have to make sure this is always correct though --
if the PC is incremented before the NULLCHECK it would not be.
Chris> 2) a new macro for array bounds checking, ARRAYBOUNDSCHECK
Chris> it also pushes the SAVE_PC into the throw block.
Chris> the code uses a clever unsigned arithmetic trick
Chris> I read in the PowerPC Compiler Writer's Guide.
This is a nice trick that gcj already uses when it generates code.
Chris> Run the cm3 benchmark on a stock 3.0.3. The performance will be
Chris> 644. Comment out the insn_nop block and move the insn_nop
Chris> label in front of the first NEXT_INSN. Very simple, very dumb
Chris> optimization. Performance goes down to 600. Attached is a
Chris> bad.PATCH file for this.
I don't have any useful insight here :-(
I've rewritten your patch to only use ARRAYBOUNDSCHECK and to add the
missing NULLCHECK. I'm going to check that in now. I've appended the
new patch. We can always reintroduce ARRAYNULLCHECK or whatever if
the problems can be solved and it makes a difference; meanwhile the
appended is a clear improvement.
I consider your patch to be below the "needs paperwork" limit, since
much of the patch is mechanical substitution. However, if you're
planning to send more libgcj patches, it would be best to get started
on the paperwork soon. Sometimes that step takes a long time :-(.
Paperwork is required if you go over some minimum patch size/quantity.
Tom
Index: ChangeLog
from Chris Sears <cbsears_sf@yahoo.com>
* interpret.cc (ARRAYBOUNDSCHECK): New macro.
(continue1) [insn_iaload, insn_laload, insn_faload, insn_daload,
insn_aaload, insn_baload, insn_caload, insn_saload, insn_iastore,
insn_lastore, insn_fastore, insn_dastore, insn_aastore,
insn_bastore, insn_castore, insn_sastore]: Use it.
(continue1) [insn_arraylength]: Check for null array.
Index: interpret.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/interpret.cc,v
retrieving revision 1.27
diff -u -r1.27 interpret.cc
--- interpret.cc 2001/10/16 08:35:17 1.27
+++ interpret.cc 2002/01/08 19:04:03
@@ -1,6 +1,6 @@
// interpret.cc - Code for the interpreter
-/* Copyright (C) 1999, 2000, 2001 Free Software Foundation
+/* Copyright (C) 1999, 2000, 2001 , 2002 Free Software Foundation
This file is part of libgcj.
@@ -187,6 +187,13 @@
do { if ((X)==NULL) throw_null_pointer_exception (); } while (0)
#endif
+#define ARRAYBOUNDSCHECK(array, index) \
+ do \
+ { \
+ if (((unsigned) index) >= (unsigned) (array->length)) \
+ _Jv_ThrowBadArrayIndex (index); \
+ } \
+ while (0)
// this method starts the actual running of the method. It is inlined
// in three different variants in the static methods run_normal,
@@ -958,10 +965,7 @@
jint index = POPI();
jintArray arr = (jintArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHI( elements(arr)[index] );
}
NEXT_INSN;
@@ -972,10 +976,7 @@
jint index = POPI();
jlongArray arr = (jlongArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHL( elements(arr)[index] );
}
NEXT_INSN;
@@ -986,10 +987,7 @@
jint index = POPI();
jfloatArray arr = (jfloatArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHF( elements(arr)[index] );
}
NEXT_INSN;
@@ -1000,10 +998,7 @@
jint index = POPI();
jdoubleArray arr = (jdoubleArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHD( elements(arr)[index] );
}
NEXT_INSN;
@@ -1014,10 +1009,7 @@
jint index = POPI();
jobjectArray arr = (jobjectArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHA( elements(arr)[index] );
}
NEXT_INSN;
@@ -1028,10 +1020,7 @@
jint index = POPI();
jbyteArray arr = (jbyteArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHI( elements(arr)[index] );
}
NEXT_INSN;
@@ -1042,10 +1031,7 @@
jint index = POPI();
jcharArray arr = (jcharArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHI( elements(arr)[index] );
}
NEXT_INSN;
@@ -1056,10 +1042,7 @@
jint index = POPI();
jshortArray arr = (jshortArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
PUSHI( elements(arr)[index] );
}
NEXT_INSN;
@@ -1171,10 +1154,7 @@
jint index = POPI();
jintArray arr = (jintArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1186,10 +1166,7 @@
jint index = POPI();
jlongArray arr = (jlongArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1201,10 +1178,7 @@
jint index = POPI();
jfloatArray arr = (jfloatArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1216,10 +1190,7 @@
jint index = POPI();
jdoubleArray arr = (jdoubleArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1231,10 +1202,7 @@
jint index = POPI();
jobjectArray arr = (jobjectArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
_Jv_CheckArrayStore (arr, value);
elements(arr)[index] = value;
}
@@ -1247,10 +1215,7 @@
jint index = POPI();
jbyteArray arr = (jbyteArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1262,10 +1227,7 @@
jint index = POPI();
jcharArray arr = (jcharArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -1277,10 +1239,7 @@
jint index = POPI();
jshortArray arr = (jshortArray) POPA();
NULLCHECK (arr);
- if (index < 0 || index >= arr->length)
- {
- _Jv_ThrowBadArrayIndex (index);
- }
+ ARRAYBOUNDSCHECK (arr, index);
elements(arr)[index] = value;
}
NEXT_INSN;
@@ -2229,6 +2188,7 @@
SAVE_PC;
{
__JArray *arr = (__JArray*)POPA();
+ NULLCHECK (arr);
PUSHI (arr->length);
}
NEXT_INSN;
More information about the Java-patches
mailing list