for review (XXL): 6655638 method handles for invokedynamic

Wed Mar 11 19:10:08 PDT 2009

I've done all the easy stuff and I'm onto the methodHandles* files but  
I thought I'd give you these first.

MethodHandleSupport isn't a great flag name since it sounds like a  
thing.  SupportMethodHandles, EnableMethodHandles or  
AllowMethodHandles would read better I think.

cppInterpreter.cpp:

method_entry(invoke_method) should be method_entry(method_handle) I  
think.

assembler_sparc.cpp:

I'd be tempted to put the delay slot nop in  
jump_to_method_handle_entry instead of expecting the caller to do it.   
Otherwise I think it needs to a comment mentioning the need to fill  
the delay.  I know the other jump_to leave it up to the caller but  
they are all pretty much single instruction.   
jump_to_method_handle_entry does a bit of work.  Either way is fine.

interpreter_sparc.cpp:

this is a pretty bizarre way to call throw_if_not_2

  __ throw_if_not_x(Assembler::never,

I know it's used in other places, I think it would be cleaner to just  
inline the needed throw_if_not_2 code.  It's just 2 assembly  
instructions.  I find the juxtaposition of if and never a little  
jarring when in fact it means always throw.

stubGenerator_x86_32.cpp:

What's this for:

+#undef  __
+#define __ _masm->
+

dictionary.hpp:

SymbolPropertyEntry and SymbolPropertyTable could use at least a small  
comment describing their intended purpose.

javaClasses.cpp:

all of the variants of this:

+  assert(Klass::cast(mname->klass())- 
 >is_subclass_of(SystemDictionary::MemberName_klass()), "wrong type");

could simply be:

  assert(is_instance(mname), "wrong type");

methodOop.hpp:

method_type_pointer_chase could use some comments.  maybe a different  
name would be better since it's not actually a pointer chase but the  
offsets to use in a pointer chase.  method_type_offsets_chain?

What units is methodOopDesc::extra_stack() in?  It says slots but to  
me that means slots in the vmreg/optoreg sense which is 32 bit words  
but I guess it means interpreter slots.  It should have a more precise  
comment and maybe the name could reflect it somehow.  I guess it's in  
the same units as max_stack and max_locals.  Maybe  
extra_stack_entries() would get that across.

The methodHandles part is to come.

tom

On Mar 10, 2009, at 3:48 PM, John Rose wrote:

> Thanks for the reviews so far.  I've posted the remainder of  
> meth.patch here:
>
> 6655638: dynamic languages need method handles
>  http://cr.openjdk.java.net/~jrose/6655638/webrev.00/  (not yet  
> integrated)
>
> (Long live cr.OJN!  Goodbye webrev.invokedynamic.info.)
>
> Later today (probably) I will update the index page to have useful  
> comments about how each file is changed.
>
> After responding to any further reviewer comments, I'll push the  
> results.
>
> Because it's a large body of code, and it will help to have a record  
> of what we do, let's keep review comments public.
>
> If the comments lead to large changes, I may have to ask for a re- 
> review.  If that happens, I'll publish at least a diff of webrev.N  
> to webrev.N+1, maybe even a 2nd-order webrev, if I can figure out how.
>
> About self-review:  I will be reviewing my own code, too, and if the  
> review period is long enough, may wish to make amendments.  I'll  
> email a public comment if this seems necessary.  (I've "discovered"  
> that people don't like it when they encounter unexplained changes  
> mid-review.)
>
> Meanwhile, I'm working on the same code that's being reviewed.  For  
> big reviews like this one, I'll try to keep separate workspaces for  
> review and further engineering.  (I wouldn't attempt this without hg/ 
> MQ and Emacs diff-mode.)  Therefore, if a review takes a long time  
> (which this one has) it may be followed immediately by a second  
> review cycle with a new bug ID, to push the adjustments separately.   
> In fact, I'll allocate the bug ID now:
>
> 6815692: method handle code needs some cleanup (post-6655638)
>  http://cr.openjdk.java.net/~jrose/6815692/webrev.00/  (not yet  
> posted)
>
> As that settles down I will be asking for reviews on this next step:
>
> 6655646: dynamic languages need dynamically linked call sites
>  http://cr.openjdk.java.net/~jrose/6655646/webrev.00/  (not yet  
> posted)
>
> After that, there will be individual pushes for additional assembly  
> code (x64 and/or sparc), compiler optimizations (2-3 steps there),  
> and various smaller things like compressed oops and cppInterpreter  
> support.  And of course bug fixes and cleanups as needed.
>
> Onward!
>
> -- John
>
> On Jan 20, 2009, at 2:53 AM, John Rose wrote:
>
>> This is part of the JVM side of the JSR 292 Reference  
>> Implementation.  It should have no effect on application execution,  
>> unless one of the new flags is turned on (chiefly -XX: 
>> +MethodHandleSupport).
>>
>> Present limitations:
>> - It works only on x86/32.  (No sparc, compressed oops, cpp  
>> interpreter.)
>> - There are no compiler optimizations.
>> - It is young code.  There are bugs.  But only when a new flag is  
>> turned on.
>>
>> This review is for contents of meth.patch, to be pushed from mlvm  
>> to http://hg.openjdk.java.net/jdk7/hotspot-comp-gate/hotspot .
>>
>> Here is the webrev for this review:
>> http://webrev.invokedynamic.info/jrose/6655638.meth/
>>
>> Here is the mlvm patch where the code currently lives:
>> http://hg.openjdk.java.net/mlvm/mlvm/hotspot/raw-file/tip/meth.patch
>>
>> An earlier version of these changes pass JPRT; it is expected that  
>> at most minor tweaks will be needed to push it through again.
>>
>> Even though they are large, the changes should also pass a simple  
>> visual inspection, since all substantially new control paths are  
>> guarded by tests of the new flags.
>>
>> Please give it a look.
>>
>> -- John
>>
>> P.S.  Here are some relevant conversations about method handles and  
>> invokedynamic:
>> http://mail.openjdk.java.net/pipermail/mlvm-dev/2009-January/000321.html
>> http://blogs.sun.com/jrose/entry/international_invokedynamic_day
>> http://blogs.sun.com/dannycoward/entry/firing_up_the_engines_for
>> http://groups.google.com/group/jvm-languages/browse_thread/thread/a4b8a616eb987ca8
>>
>> P.P.S. The proposed JDK changes are independent.  At present, you  
>> can find them here, in patch and webrev formats:
>> http://hg.openjdk.java.net/mlvm/mlvm/hotspot/raw-file/tip/meth.proj.patch
>> http://webrev.invokedynamic.info/jrose/6655638.meth.proj/
>