Fwd: [jsr-292-eg] updated RI code and API javadoc

[Rémi Forax]

John Rose a écrit :
> Dear JSR 292 Observers and MLVM members,
>
> Daniel Heidinga of the IBM VM team has sent some excellent 
> comments on the 
> recent draft APIs.  Some of you who are working with the Reference 
> Implementation or the JSR 292 
> Backport may find that  his comments ring a bell for you, or you may 
> have a use case (or abuse case!) for one of the features we are trying 
> to decide on.  Please weigh in!
>
> -- John

Ok,


>
> Begin forwarded message:
>
> *From: *Daniel Heidinga <Daniel_Heidinga at ca.ibm.com 
> <mailto:Daniel_Heidinga at ca.ibm.com>>
> *Date: *July 8, 2009 1:39:36 PM PDT
> *To: *jsr-292-eg at jcp.org <mailto:jsr-292-eg at jcp.org>
> *Subject: **Re: [jsr-292-eg] updated RI code and API javadoc*
> *Reply-To: *jsr-292-eg at jcp.org <mailto:jsr-292-eg at jcp.org>
>
> We've taken a look through the Javadoc and have some comments.
>
> ==== Overall Comments ====
> - There are references to sun.dyn.* classes in the documentation which 
> need to be removed. All classes that are part of the spec must be in a 
> java.dyn package.
>
Agree, Christian has started to update the RI.

> - We still oppose adding "throws Throwable/Exception" to all 
> invokedynamic instructions. We have yet to see a valid reason for this 
> presented on the mailing list.
>

"throws Throwable", it is only at Java side, not at VM side.

> - MethodHandle.Lookup and the new Modularity JSR - has any thought 
> been given to how these will interact?
>
Good question. I suppose that when the public API part of jigsaw will be 
integrated in the JDK7,
the RI will be updated to check module access.

> - Links to blogs, etc need to be replaced with the actual content. 
> This will keep the spec and the content in sync.
>
Agree
>
>
> ==== CallSite ====
> * Cannot be a subclass of sun.dyn.CallSiteImpl
> * initialTarget() method missing from javadoc. We like the 
> initialTarget() method as it provides a "fallback" method for HCR and 
> invalidation.
>

What HCR means. Currently initialTarget is called if setTarget() is not 
called in boostrap method, i.e
during the time the call site is created and the time the callsite is 
populated with VM data.

> * Contradiction regarding "state is never null" and "If the bootstrap 
> method does not assign a non-null value to the new call site's target 
> variable, the method initialTarget()"
>
target can be null until the VM initialise the call site.

> * CallSite(j.l.Object caller, j.l.String, MethodType)
> * caller parameter and return value of callerClass() do not match
> * Two options:
> 1. Fix CallSite constructor so that caller parameter is a j.l.Class.
> 2. Take an "invalidationToken" rather than the j.l.Class caller as a 
> parameter.
> Change callerClass() to j.l.Object invalidationToken() and use this 
> value in the invalidation logic when invalidating.
> Change Linkage.invalidateCallerClass(j.l.Class callerClass) to 
> Linkage.invalidateToken(j.l.Object token).
> No difference to the VM whether it is holding onto a class or an object.
>

As the backport implementer,
letting the languege developer controlling the invalidation token
is not appealing. Because this means that the runtime has to store 
associations between
invalidation tokens and callsites.

> * nameComponents() and name()
>

In my opinion, nameComponents() should be removed. I don't see clearly 
why JSR292 need to specify that.

> * both link to your blog. Any relevant information there needs to be 
> moved to the javadoc so it is kept in sync with the doc.
>
Agree.
>
> * callerClass()
> * can't return a class if CallSite takes an Object
> * name()
> * blog link contents need to be defined in the JavaDoc.
> * "dangerous" characters missing '<', '>', ';', '['. '<' & '>' because 
> its a method name. Better to be exact when the list is so short.
> * nameComponents()
> * blog link contents need to be defined in the JavaDoc.
>
Agree.
>
> * This feels a lot like an implementation detail.
> * toString()
> * Either document the format or don't override Object's 
> implementation. User's may parse the output so all VM's should return 
> the same format or nothing at all.
>
Agree.


I also propose to slim the object CallSite.
The only required field is method type to be able to do a check when 
calling setTarget()
The caller class and the name of the method name are not necessary
because the bootstrap method already provides these info.

So I propose to split CallSite in two, CallSite and StandardCallSite,
CallSite with a constrcutor that takes a MethodType and StandardCallSite 
that inherits
from CallSite and takes a method type, a method name and a caller class.


>
> ==== InvokeDynamic ====
> * "a one-to-one association" - what about call site splitting?
>
Agree. Too restrictive.

>
> ==== JavaMethodHandle ====
> * Please provide a use case for this. We still don't see a need for 
> this and don't want to see it as API unless there is a strong reason 
> to add it.
> * spec'd in terms of syn.dyn.* classes - Needs to only refer to 
> java.dyn classes.
> * Please document the constructors for this class as well as any other 
> public API.
>
As I already said, I am not happy with the method lookup as specified
in constructors that take a method name.

I am not sure JavaMethodHandle is required. Correct me If I'm wrong but
you can do exactly the same thing with lookup().bind(...).

>
> ==== Linkage ====
> * registerBootstrapMethod(j.l.Class, j.l.String)
> * Why is the j.l.Class parameter called "runtime" in this method and 
> "callerClass" in registerBootstrapMethod(j.l.Class, MethodHandle)?
> * Do we really need three versions of registerBootstrapMethod? Lets 
> aim for a small, simple API with as few unnecessary helpers as possible.
>

registerBootstrapMethod is often called in the static block of a class 
containing invokedynamic called.
So the two versions register(String), register(Class<?>, String) are 
convenient method that avoid
to create a MethodHandle in the static block thus simplify the work of 
compiler writer.

These two methods can be removed if the LDC method handle is introduced
by the way, I don't like it because a method handle is not a constant.

So I agree with you about the fact that the API should be small but
these two calls really worth their weight.


> * invalidateCallerClass()
> * invalidateToken proposal as CallSite takes an Object, not a Class. 
> See notes on CallSite.
> * invalidateAll()
> * "It is unspecified whether call sites already known to the Java code 
> will continue to be associated with invokedynamic instructions. If any 
> call site is still so associated, its CallSite.getTarget() method is 
> guaranteed to return null the invalidation operation completes." - 
> This needs to be defined. Otherwise, there will be issues with 
> different VMs operating differently.
> * invalidateCallerClass() & invalidateAll()
>

These words seam to predate the fact that now call site target can't be 
null (apart during initialisation).
Invalidation is tricky, because it can trigger deoptimisation of call site.
I propose the same kind of wording that the one about class transformation:
see 
http://download.java.net/jdk7/docs/api/java/lang/instrument/Instrumentation.html#retransformClasses%28java.lang.Class...%29

If a invalidated call site is used by active stack frames, those active 
frames continue to call the old target method handle.
The call site initialTarget() method handle will be used on new invokes.

> * why do these methods have a return value instead of void?
>
Good question.

>
> ==== MethodHandle ====
> * Need to remove "subclasses syn.dyn.MethodHandleImpl"
> * "Every invoke method always throws Exception" - We want to see a 
> valid reason for this. Otherwise, strongly opposed!
> * "ldc instruction which refers to a CONSTANT_Methodref or 
> CONSTANT_InterfaceMethodref" - This feels like a hack. See our LDC 
> proposal (sent 06/17/2009) for adding CONSTANT_MethodHandleref_info & 
> CONSTANT_MethodTyperef_info ConstantPool entries.
> * Please format the examples to be more readable by adding blank lines.
>
> ==== MethodHandles ====
> * Lots of new "throws Throwable" - Can you provide the reasoning for 
> this? Why Throwable not Exception? If this is due to the project coin 
> requests, please provide background info as we oppose the change.
> * Any method using conversions from convertArguments() should also 
> throw WrongMethodTypeException if the conversions fail: 
> permuteArguments, spreadArguments, etc
>

Agree. But I woul prefer IllegalArgument exception instead of 
WrongMethodTypeException.
And let WrongMethodTypeException only raised when doing a 
MethodHandle.invoke().

> * There seems to be a lot of new methods: catchException, 
> throwException, dynamicInvoker, filterArguments, invoke_5-9, 
> methodType*, varargsInvoker, publicLookup
> * This seems to be a strong argument in favour of Fredrik's proposal - 
> we won't be able to anticipate all of the useful adaptors down the road.
>

I like Fredrik's proposal but how to check efficiently that a method 
handle fit a method type.
Until someone comes with a way to answer to that question, the proposal 
will stay a proposal.

publicLookup() was Lookup.PUBLIC_LOOKUP.
see http://www.mail-archive.com/mlvm-dev@openjdk.java.net/msg00663.html

I agree that methodType* should be removed.
see http://www.mail-archive.com/mlvm-dev@openjdk.java.net/msg00706.html

>
> * invoke_0(MethodHandle), invoke_1(MethodHandle, j.l.Object), ...., 
> invoke(MethodHandle, j.l.Object ...)
> * Why not take a page from Smalltalk naming conventions and call these 
> methods
> invoke(MethodHandle)
> invoke(MethodHandle, j.l.Object)
> and finally,
> invokeWithArguments(MethodHandle, j.l.Object[])?
> This seems much cleaner then having 10 invoke_# methods.
> * This might be a good place to prune our API as well. Do we really 
> need 10 of these invoke_# methods?
>

This method are usefull if you use a specific boostrap method for by 
example binary method (like operators)
and want to installa a fast path depending on the class of the two 
arguments.
In that case, you will first install a method handle that get the 
classes of the arguments
and install (setTarget) a specific fast path. Because this method handle 
need to return a value and
you now that this is a binary method, you can use invoke_2.

>
> * genericInvoker()
> * Seems cleaner in this revision.
> * It appears that the behaviour from genericInvoker() has been split 
> between genericInvoker() and varargsInvoker(). Is this correct?
>
yes
>
> * dynamicInvoker()
> * This is only correct if Fredrik's proposal is ignored - otherwise, 
> the invoker in the "equivalent to the following code" should not be an 
> exactInvoker.
> * convertArguments()
> * The conversion rules appear much cleaner.
> * permuteArguments()
> * Should throw WrongMethodTypeException if conversions fail. 
> ("Pairwise conversions are applied.... as with convertArguments")
>
Agree.
>
> * collectArguments()
> * "collecting a series of trailing arguments as arguments to a 
> pre-processor method" - What "pre-processor method"? Can you either 
> define or remove the sentence?
> * dropArguments()
> * Some of the examples seem wrong - // xy and // yz specifically.
> * // xy should be // yz
> * // yz should be // xy
>
already fixed.
>
> * catchException()
> * Scary "(how? TBD)" regarding passing args to the handler.
> * Are changes to the original args by the target reflected in the 
> values passed to the handler when the exception occurs? Or are they a 
> pristine copy of the args as sent to target MH?
> * Idea is ok
>
I think, it should work like the snippet of code given in the doc.

> * throwException()
> * Seems pointless.
> * Can easily be done without having a special adapter for it.
> * Do you have a valid use case for this? Otherwise, we should remove it.
>
Agree.

> * methodType()
> * Please remove these methods. They are needless helpers and lead to 
> API bloat.
>
Agree.
>
>
>
> ==== MethodHandles.Lookup ====
> * Still lots of TBD
> * How does this play with the new modularity jsr?
> * new methods: in
>
> * in(j.l.Class)
> * "guaranteed to have no more access privileges than the original." 
> Unclear - please clarify.
> * Can you provide a use case?
> * toString()
> * Please document the format or remove. All JVM's should have the same 
> format.
> * findVirtual()
> * "BUG NOTE" RI implementation problem. Not general issue.
>

Rémi