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

[Charles Oliver Nutter]

Do you want comments in a specific form? I can provide use-driven
justifications for several cases he brings up.

On Wed, Jul 8, 2009 at 10:37 PM, John Rose<John.Rose at sun.com> wrote:
> 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
>
> Begin forwarded message:
> From: Daniel Heidinga <Daniel_Heidinga at ca.ibm.com>
> Date: July 8, 2009 1:39:36 PM PDT
> To: 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
>
> 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.
> - 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.
> - MethodHandle.Lookup and the new Modularity JSR - has any thought been
> given to how these will interact?
> - Links to blogs, etc need to be replaced with the actual content. This will
> keep the spec and the content in sync.
>
> ==== 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.
> * 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()"
> * 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.
> * nameComponents() and name()
> * 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.
> * 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.
> * 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.
>
> ==== InvokeDynamic ====
> * "a one-to-one association" - what about call site splitting?
>
> ==== 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.
>
> ==== 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.
> * 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()
> * why do these methods have a return value instead of void?
>
> ==== 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
> * 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.
>
> * 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?
>
> * genericInvoker()
> * Seems cleaner in this revision.
> * It appears that the behaviour from genericInvoker() has been split between
> genericInvoker() and varargsInvoker(). Is this correct?
> * 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")
> * 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
> * 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
> * 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.
> * methodType()
> * Please remove these methods. They are needless helpers and lead to API
> bloat.
>
>
> ==== 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.
>
>
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>
>