Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/24/2012 07:15 PM, Christian Thalinger wrote: >> Not sure if this logic is applicable in this particular case. This is >> the potential "performance cliff" you are eager to get rid of with new >> implementation. > > No it's not. We know exactly what causes the performance cliff. It's a > completely unrelated issue in the VM. Sorry for misguided definition there, thought quoting does the trick for me. I was meant to say that the scalability bottlenecks like these pop out quickly, unexpected and hit hard. The difference between single-threaded and two-threaded versions could be so dramatic, so you can easily say it goes down the sink. It's inconvenient to fix one "cliff" and introduce the other, albeit "completely unrelated". -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 24, 2012, at 2:55 AM, Aleksey Shipilev wrote: > On 07/23/2012 10:31 PM, John Rose wrote: >> On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: >> The code does not need to be scalable, because the number of entries in >> the cache is small (order of 10-100) and scales only with type schema >> complexity, not workload complexity. > > If I had a nickel... > > Not sure if this logic is applicable in this particular case. This is > the potential "performance cliff" you are eager to get rid of with new > implementation. No it's not. We know exactly what causes the performance cliff. It's a completely unrelated issue in the VM. -- Chris > Given enough users and applications make use of this > code, someone will eventually step and burn on this. > > This wishful thinking "it's okay to use synchronized here, because this > couldn't possibly get contended" lead to many beautiful scalability > bottlenecks throughout the JDK. While it is usually a simple thing to > fix, I'm keen on not allowing to make the same mistakes over and over again. > >> So in this case, "static synchronized" is the correct choice. > > I shall wait for mainline integration to complete and then try to run > the microbenchmarks against the new backend; will see if this potential > issue is the practical one. > > -Aleksey. > > ___ > mlvm-dev mailing list > mlvm-dev@openjdk.java.net > http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/23/2012 10:31 PM, John Rose wrote: > On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: > The code does not need to be scalable, because the number of entries in > the cache is small (order of 10-100) and scales only with type schema > complexity, not workload complexity. If I had a nickel... Not sure if this logic is applicable in this particular case. This is the potential "performance cliff" you are eager to get rid of with new implementation. Given enough users and applications make use of this code, someone will eventually step and burn on this. This wishful thinking "it's okay to use synchronized here, because this couldn't possibly get contended" lead to many beautiful scalability bottlenecks throughout the JDK. While it is usually a simple thing to fix, I'm keen on not allowing to make the same mistakes over and over again. > So in this case, "static synchronized" is the correct choice. I shall wait for mainline integration to complete and then try to run the microbenchmarks against the new backend; will see if this potential issue is the practical one. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
I think it is good. Vladimir John Rose wrote: > On Jul 23, 2012, at 11:15 AM, Vladimir Kozlov wrote: > >> Both webrevs point to jdk changes. Where are hotspot changes? > > Oops, fixed. Please try again: > > http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ > http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ > > — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 23, 2012, at 11:15 AM, Vladimir Kozlov wrote: > Both webrevs point to jdk changes. Where are hotspot changes? Oops, fixed. Please try again: http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: > Global synchronization is the performance smell, and this looks to be > potential scalability bottleneck (it sends shivers down my spine every > time I see "static synchronized" in the same line. That is not to > mention synchronizing on placeholder looks suspicious and error-prone. I believe the code is correct. The purpose is to load as many distinct "species" of BMH tuples as the application type schema requires. The use of placeholders is patterned after the JVM's internal class loading algorithm. The code does not need to be scalable, because the number of entries in the cache is small (order of 10-100) and scales only with type schema complexity, not workload complexity. So in this case, "static synchronized" is the correct choice. > Do you need help rewriting this with CHM? No, but thanks for the help! — John___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
John, Both webrevs point to jdk changes. Where are hotspot changes? Vladimir John Rose wrote: > On Jul 13, 2012, at 2:41 AM, John Rose wrote: > >> Here is that webrev: >> http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ >> >> These are the changes to JDK code that accompany the JVM changes >> already under review. > > I have updated both webrevs to their final contents, as follows: > > http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ > http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ > > In the same folder are incremental webrevs, for the record. > > Thanks to all reviewers for their help. > > — John > > P.S. If there's something you don't like in one of the files, let us know. > As I noted before, we can (and will) roll more adjustments into the next > push. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 04:16 AM, John Rose wrote: > P.S. If there's something you don't like in one of the files, let us know. > As I noted before, we can (and will) roll more adjustments into the next > push. I have a question about $PREPARED_FORMS there. It looks like it is not used anywhere in the code, should we eliminate it whatsoever? If not, then I have trouble understanding if we need to explicitly override CHM defaults, especially concurrency level there. (I know Doug pushes back on specialized constructors in CHMv8 because some of the parameters are meaningless there). -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 03:45 AM, John Rose wrote:
> On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote:
> Yes.
Thanks John. I'm having a glance over the fix in new webrev, and this
feels even worse:
+ private static SpeciesData get(String types) {
+// Acquire cache lock for query.
+SpeciesData d = lookupCache(types);
+if (!d.isPlaceholder())
+return d;
+Class cbmh;
+synchronized (d) {
+// Use synch. on the placeholder to prevent multiple
instantiation of one species.
+// Creating this class forces a recursive call to
getForClass.
+cbmh = Factory.generateConcreteBMHClass(types);
+}
+// Reacquire cache lock.
+d = lookupCache(types);
+// Class loading must have upgraded the cache.
+assert(d != null && !d.isPlaceholder());
+return d;
+}
+ static SpeciesData getForClass(String types, Class clazz) {
+// clazz is a new class which is initializing its
SPECIES_DATA field
+return updateCache(types, new SpeciesData(types, clazz));
+}
+ private static synchronized SpeciesData lookupCache(String types) {
+SpeciesData d = CACHE.get(types);
+if (d != null) return d;
+d = new SpeciesData(types);
+assert(d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
+ private static synchronized SpeciesData updateCache(String types,
SpeciesData d) {
+SpeciesData d2;
+assert((d2 = CACHE.get(types)) == null || d2.isPlaceholder());
+assert(!d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
Global synchronization is the performance smell, and this looks to be
potential scalability bottleneck (it sends shivers down my spine every
time I see "static synchronized" in the same line. That is not to
mention synchronizing on placeholder looks suspicious and error-prone.
Do you need help rewriting this with CHM?
-Aleksey.
signature.asc
Description: OpenPGP digital signature
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote: > This feels outright wrong: > > +static Map CACHE = new IdentityHashMap<>(); > I couple of questions pops out in my mind: > 1. Is this code supposed to be thread-safe? It is then wrong to use > unguarded IdentityHashMap without external synchronization. > 2. Do we really need a second .get() check, if code is not thread-safe > anyway? This code allows two threads to call put() on the same key anyway. > 3. Do the $types *need* to be interned? Or is this the premature > optimization (gone wrong, btw)? You are right on all points. Fixed. > I would rather like to see CHM.putIfAbsent()-based cache on plain > non-interned strings there. Even if interned strings are required, we > could intern them on successful map update, not every time we look up > the key. Yes. Using a IdentityHashMap here is an anti-pattern; sorry to propagate the bad example. Thanks, — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 13, 2012, at 2:41 AM, John Rose wrote: > Here is that webrev: > http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ > > These are the changes to JDK code that accompany the JVM changes already > under review. I have updated both webrevs to their final contents, as follows: http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ In the same folder are incremental webrevs, for the record. Thanks to all reviewers for their help. — John P.S. If there's something you don't like in one of the files, let us know. As I noted before, we can (and will) roll more adjustments into the next push.___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 14, 2012, at 5:43 AM, Rémi Forax wrote:
> On 07/14/2012 02:02 PM, David Schlosnagle wrote:
>> John,
>
> Hi David,
> I hijack the thread …
Thanks for the comments, Remi & David.
>> src/share/classes/java/lang/invoke/BoundMethodHandle.java
>>
>> On lines 131-132: Just out of curiosity, why use pos+1 in one line
>> versus 1+pos on the next? Would it be worthwhile to extract pos+1 to a
>> local int end to save some bytecode?
> Usually you should not care about this kind of change, micro-optimizing
> is useless.
> Or this code is not called enough and you don't care or this code is hot
> and
> the JIT will do what should be done and because the JIT may inline
> the code of dropParameterTypes and bind(), it will better optimize that you
> because it will be able to share more sub-expressions.
Remi is correct.
>> default case should never be called, so don't 'optimize' for it.
Yes.
>>
>> On line 464: Is there any reason CACHE should not be final?
>> static final Map CACHE = new IdentityHashMap<>();
>
> this one may be important, static final => const for the VM
Yes, that was an oversight. Fixed.
>> On lines 473-486: Do the accesses and mutations of CACHE need to be
>> thread safe or are the uses assumed to be safe by other means?
This was a bug. We have added appropriate synchronization.
>> On lines 778-784: Should this use types.charAt(int) rather than
>> toCharArray() to copy the array?...
>
> ...Anyway, the real question here, how often this code will be called and
> even if this code is called often, it's perhaps not a bottleneck.
It's not a bottleneck. BMH species are few in number.
>> Would it be worthwhile for clarity to define constants to represent
>> the BaseTypes ('B', 'C', 'D', 'F', 'I', 'J', 'S', 'Z') and ObjectType
>> ('L') per JVM spec section 4.3.2 for use throughout these
>> java.lang.invoke.* implementation classes (e.g. the various
>> switch/case in BoundMethodHandle, and DirectMethodHandle.bindArgument,
>> throughout LambdaForm)? I assume that anyone touching this code would
>> be familiar with these types so constants may actually reduce clarity
>> for some developers.
That is my sense of it. The only value to making named constants here would be
a slight increase in static checking, but I don't think it's worthwhile.
>> src/share/classes/java/lang/invoke/CountingMethodHandle.java
>>
>> On line 40: Should instance field target be final?
>> private final MethodHandle target;
>
> yes.
Even better: During the review I have been finding more dead code to
eliminate. This whole class has been finalized, in the sense of "deleted".
>> src/share/classes/java/lang/invoke/Invokers.java
>>
>> On lines 322-326: Is this err.initCause(ex) needed since the cause is
>> already passed to the 2 arg InternalError constructor (maybe leftover
>> from before the constructor was added in JDK8)?
It is a leftover. Thanks for spotting it.
>> src/share/classes/java/lang/invoke/MemberName.java
>>
>> On lines 81-90 there is an implementation of equals(Object), but on
>> lines 593-603 there is a commented out implementation of
>> equals(Object) and hashCode(). Are the commented out versions for
>> future usage? There should probably be an implementation of hashCode()
>> consistent with equals, although MemberName may not be used in a hash
>> based structure. Also, it might be worthwhile to collocate the "TO BE"
>> implementations with near the current ones with a comment.
This was an oversight due to some hasty coding. Thanks, fixed.
>>
>> On lines 268-278: Should the isObjectPublicMethod() method also match
>> public Object methods getClass(), notify(), notifyAll(), wait(),
>> wait(long), and wait(long, int) or are those not intended to be used?
No, those aren't relevant. Since the method is private and its name
is long enough already, I'm leaving it as is.
Thanks again!
— John
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
(sorry for not replying to original message, just subscribed)
This feels outright wrong:
+static Map CACHE = new IdentityHashMap<>();
+
+static Data get(String types) {
+final String key = types.intern();
+Data d = CACHE.get(key);
+if (d == null) {
+d = make(types);
+Data e = CACHE.get(key);
+if (e != null) {
+d = e;
+} else {
+CACHE.put(key, d);
+}
+}
+return d;
+}
I couple of questions pops out in my mind:
1. Is this code supposed to be thread-safe? It is then wrong to use
unguarded IdentityHashMap without external synchronization.
2. Do we really need a second .get() check, if code is not thread-safe
anyway? This code allows two threads to call put() on the same key anyway.
3. Do the $types *need* to be interned? Or is this the premature
optimization (gone wrong, btw)?
I would rather like to see CHM.putIfAbsent()-based cache on plain
non-interned strings there. Even if interned strings are required, we
could intern them on successful map update, not every time we look up
the key.
-Aleksey.
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/14/2012 02:02 PM, David Schlosnagle wrote:
> On Fri, Jul 13, 2012 at 5:41 AM, John Rose
> wrote:
>> On Jul 11, 2012, at 5:53 PM, John Rose wrote:
>>
>>> As some of you have noticed, Chris Thalinger, Michael Haupt, and I
>>> have been working on the mlvm patches [1] for JEP-160 [2] for
>>> several months. These changes make method handles more
>>> optimizable. They refactor lots of "magic" out of the JVM and into
>>> more manageable Java code.
>>> …
>>> An associated webrev for hotspot-comp/jdk/ will be posted soon; it
>>> is already present on mlvm-dev for the curious to examine. (This
>>> change set also deletes a lot of old code.)
>> Here is that webrev:
>> http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/
>>
>> These are the changes to JDK code that accompany the JVM changes
>> already under review.
>>
>> There are 2900 LOC deleted, and 7000 LOC added. Key changes:
>> - method handle behavior is fully represented by LambdaForm objects
>> - chained method handles (including "adapter method handles") are gone
>> - an ASM-based bytecode spinner compiles LambdaForms when they warm up
>> - bound method handles are compactly represented without boxing
>> - the private symbol-resolution interface to the JVM (MemberName)
>> is improved
>> - unit tests have more systematic coverage
>> - a number of minor bugs are fixed
>>
>> This is implementation work. No public Java APIs are changed,
>> although the javadoc is slightly edited for clarity.
>>
>> Please have a look.
>>
>> — John
> John,
Hi David,
I hijack the thread ...
>
> I had a few quick comments on your webrev. Some of these might be
> considered premature or unnecessary optimization, so take them as you
> wish.
>
> src/share/classes/java/lang/invoke/BoundMethodHandle.java
>
> On lines 131-132: Just out of curiosity, why use pos+1 in one line
> versus 1+pos on the next? Would it be worthwhile to extract pos+1 to a
> local int end to save some bytecode?
> MethodHandle bindArgument(int pos, char basicType, Object value) {
> int end = pos + 1;
> MethodType type = type().dropParameterTypes(pos, end);
> LambdaForm form = internalForm().bind(basicType, end, tcount());
> if (basicType == 'I' && !(value instanceof Integer)) {
> // Cf. ValueConversions.unboxInteger
> value = ValueConversions.primitiveConversion(Wrapper.INT,
> value, true);
> }
> return cloneExtend(type, form, basicType, value);
> }
Usually you should not care about this kind of change, micro-optimizing
is useless.
Or this code is not called enough and you don't care or this code is hot
and
the JIT will do what should be done and because the JIT may inline
the code of dropParameterTypes and bind(), it will better optimize that you
because it will be able to share more sub-expressions.
Now just for fun, if you count the number of bytecodes, I think your
proposed code
is bigger because 'end' is the local variable 4 and the is no
istore_4/iload_4 so
the compiler will use the generic iload/istore which takes 2 bytes.
so pos + 1 is iload_1, iconst_1, iadd, so the current version use 6
bytecodes,
your version use iload_1, iconst_1, iadd, istore 4 + iload 4 * 2= 9
bytecodes
>
> On lines 202-212: Similarly would it be worthwhile to extract types()
> to a local String within internalValues()?
> final Object internalValues() {
> String types = types();
> Object[] boundValues = new Object[types.length()];
> for (int i = 0; i < boundValues.length; ++i) {
> try {
> switch (types.charAt(i)) {
> case 'L': boundValues[i] = argL(i); break;
> case 'I': boundValues[i] = argI(i); break;
> case 'F': boundValues[i] = argF(i); break;
> case 'D': boundValues[i] = argD(i); break;
> case 'J': boundValues[i] = argJ(i); break;
> default : throw new InternalError("unexpected type: "
> + types.charAt(i));
> }
> } catch (Throwable t) {
> throw new InternalError(t);
> }
> }
> return Arrays.asList(boundValues);
> }
default case should never be called, so don't 'optimize' for it.
>
> On line 464: Is there any reason CACHE should not be final?
> static final Map CACHE = new IdentityHashMap<>();
this one may be important, static final => const for the VM
>
> On lines 473-486: Do the accesses and mutations of CACHE need to be
> thread safe or are the uses assumed to be safe by other means?
>
> On lines 778-784: Should this use types.charAt(int) rather than
> toCharArray() to copy the array?
> private static String makeSignature(String types, boolean
> ctor) {
> StringBuilder buf = new StringBuilder(SIG_INCIPIT);
> for (int i = 0; i < types.length(); ++i) {
> buf.append(typeSig(types.cha
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Fri, Jul 13, 2012 at 5:41 AM, John Rose wrote:
> On Jul 11, 2012, at 5:53 PM, John Rose wrote:
>
>> As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been
>> working on the mlvm patches [1] for JEP-160 [2] for several months. These
>> changes make method handles more optimizable. They refactor lots of "magic"
>> out of the JVM and into more manageable Java code.
>> …
>> An associated webrev for hotspot-comp/jdk/ will be posted soon; it is
>> already present on mlvm-dev for the curious to examine. (This change set
>> also deletes a lot of old code.)
>
> Here is that webrev:
> http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/
>
> These are the changes to JDK code that accompany the JVM changes already
> under review.
>
> There are 2900 LOC deleted, and 7000 LOC added. Key changes:
> - method handle behavior is fully represented by LambdaForm objects
> - chained method handles (including "adapter method handles") are gone
> - an ASM-based bytecode spinner compiles LambdaForms when they warm up
> - bound method handles are compactly represented without boxing
> - the private symbol-resolution interface to the JVM (MemberName) is improved
> - unit tests have more systematic coverage
> - a number of minor bugs are fixed
>
> This is implementation work. No public Java APIs are changed, although the
> javadoc is slightly edited for clarity.
>
> Please have a look.
>
> — John
John,
I had a few quick comments on your webrev. Some of these might be
considered premature or unnecessary optimization, so take them as you
wish.
src/share/classes/java/lang/invoke/BoundMethodHandle.java
On lines 131-132: Just out of curiosity, why use pos+1 in one line
versus 1+pos on the next? Would it be worthwhile to extract pos+1 to a
local int end to save some bytecode?
MethodHandle bindArgument(int pos, char basicType, Object value) {
int end = pos + 1;
MethodType type = type().dropParameterTypes(pos, end);
LambdaForm form = internalForm().bind(basicType, end, tcount());
if (basicType == 'I' && !(value instanceof Integer)) {
// Cf. ValueConversions.unboxInteger
value = ValueConversions.primitiveConversion(Wrapper.INT,
value, true);
}
return cloneExtend(type, form, basicType, value);
}
On lines 202-212: Similarly would it be worthwhile to extract types()
to a local String within internalValues()?
final Object internalValues() {
String types = types();
Object[] boundValues = new Object[types.length()];
for (int i = 0; i < boundValues.length; ++i) {
try {
switch (types.charAt(i)) {
case 'L': boundValues[i] = argL(i); break;
case 'I': boundValues[i] = argI(i); break;
case 'F': boundValues[i] = argF(i); break;
case 'D': boundValues[i] = argD(i); break;
case 'J': boundValues[i] = argJ(i); break;
default : throw new InternalError("unexpected type: "
+ types.charAt(i));
}
} catch (Throwable t) {
throw new InternalError(t);
}
}
return Arrays.asList(boundValues);
}
On line 464: Is there any reason CACHE should not be final?
static final Map CACHE = new IdentityHashMap<>();
On lines 473-486: Do the accesses and mutations of CACHE need to be
thread safe or are the uses assumed to be safe by other means?
On lines 778-784: Should this use types.charAt(int) rather than
toCharArray() to copy the array?
private static String makeSignature(String types, boolean ctor) {
StringBuilder buf = new StringBuilder(SIG_INCIPIT);
for (int i = 0; i < types.length(); ++i) {
buf.append(typeSig(types.charAt(i)));
}
return buf.append(')').append(ctor ? "V" : BMH_SIG).toString();
}
Would it be worthwhile for clarity to define constants to represent
the BaseTypes ('B', 'C', 'D', 'F', 'I', 'J', 'S', 'Z') and ObjectType
('L') per JVM spec section 4.3.2 for use throughout these
java.lang.invoke.* implementation classes (e.g. the various
switch/case in BoundMethodHandle, and DirectMethodHandle.bindArgument,
throughout LambdaForm)? I assume that anyone touching this code would
be familiar with these types so constants may actually reduce clarity
for some developers.
src/share/classes/java/lang/invoke/CountingMethodHandle.java
On line 40: Should instance field target be final?
private final MethodHandle target;
src/share/classes/java/lang/invoke/Invokers.java
On lines 322-326: Is this err.initCause(ex) needed since the cause is
already passed to the 2 arg InternalError constructor (maybe leftover
from before the constructor was added in JDK8)?
} catch (Exception ex) {
throw new InternalError("Exception while resolving inexact
invoke", ex);
}
src/share/classes/java/lang/invoke/MemberName
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
BoundMethodHandle.java: I am concern that BMH subclass names are looks like constants names: BMH_LLI. In several places you have BMH constants and variables: + final Class BMH = BoundMethodHandle.class; + static final String BMH = "java/lang/invoke/BoundMethodHandle"; ThrowExceptionsTest.java: empty diffs ValueConversionsTest.java: remove commented print statement if it is not needed: -System.out.println(arrayType.getSimpleName()); +//System.out.println(arrayType.getSimpleName()); Vladimir John Rose wrote: > On Jul 11, 2012, at 5:53 PM, John Rose wrote: > >> As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been >> working on the mlvm patches [1] for JEP-160 [2] for several months. These >> changes make method handles more optimizable. They refactor lots of "magic" >> out of the JVM and into more manageable Java code. >> … >> An associated webrev for hotspot-comp/jdk/ will be posted soon; it is >> already present on mlvm-dev for the curious to examine. (This change set >> also deletes a lot of old code.) > > Here is that webrev: > http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ > > These are the changes to JDK code that accompany the JVM changes already > under review. > > There are 2900 LOC deleted, and 7000 LOC added. Key changes: > - method handle behavior is fully represented by LambdaForm objects > - chained method handles (including "adapter method handles") are gone > - an ASM-based bytecode spinner compiles LambdaForms when they warm up > - bound method handles are compactly represented without boxing > - the private symbol-resolution interface to the JVM (MemberName) is improved > - unit tests have more systematic coverage > - a number of minor bugs are fixed > > This is implementation work. No public Java APIs are changed, although the > javadoc is slightly edited for clarity. > > Please have a look. > > — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 11, 2012, at 5:53 PM, John Rose wrote: > As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been > working on the mlvm patches [1] for JEP-160 [2] for several months. These > changes make method handles more optimizable. They refactor lots of "magic" > out of the JVM and into more manageable Java code. > … > An associated webrev for hotspot-comp/jdk/ will be posted soon; it is already > present on mlvm-dev for the curious to examine. (This change set also > deletes a lot of old code.) Here is that webrev: http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ These are the changes to JDK code that accompany the JVM changes already under review. There are 2900 LOC deleted, and 7000 LOC added. Key changes: - method handle behavior is fully represented by LambdaForm objects - chained method handles (including "adapter method handles") are gone - an ASM-based bytecode spinner compiles LambdaForms when they warm up - bound method handles are compactly represented without boxing - the private symbol-resolution interface to the JVM (MemberName) is improved - unit tests have more systematic coverage - a number of minor bugs are fixed This is implementation work. No public Java APIs are changed, although the javadoc is slightly edited for clarity. Please have a look. — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
