for review (XXL): 6655638 method handles for invokedynamic
Thomas.Rodriguez at Sun.COM
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.
method_entry(invoke_method) should be method_entry(method_handle) I
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.
this is a pretty bizarre way to call throw_if_not_2
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.
What's this for:
+#define __ _masm->
SymbolPropertyEntry and SymbolPropertyTable could use at least a small
comment describing their intended purpose.
all of the variants of this:
>is_subclass_of(SystemDictionary::MemberName_klass()), "wrong type");
could simply be:
assert(is_instance(mname), "wrong type");
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.
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
> (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
> 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
> 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
> 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
> 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.
> -- 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:
>> Present limitations:
>> - It works only on x86/32. (No sparc, compressed oops, cpp
>> - 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:
>> Here is the mlvm patch where the code currently lives:
>> 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
>> P.P.S. The proposed JDK changes are independent. At present, you
>> can find them here, in patch and webrev formats:
More information about the hotspot-compiler-dev