After (lengthy) examination, it seems that properly addressing the synthesized parameters issue will require more changes to javac. In light of this, I think that the synthesized parameter logic should go in a follow-up enhancement. I have created JDK-8006345 to track this issue:
http://bugs.sun.com/view_bug.do?bug_id=8006345 Therefore, I've rolled back the changes I was making which would have synthesized parameters in the reflection API itself, and updated the webrev. Please examine for any additional concerns. On 01/11/13 13:27, Joe Darcy wrote: > Hi Eric, > > Taking another look at the code, some extra logic / checking is needed > in cases where the number of source parameters (non-synthetic and > non-synthesized) disagrees with the number of actual parameters at a > class file level. > > For example, if the single source parameter of an inner class > constructor is annotated, the annotation should be associated with the > *second* parameter since the first class file parameter is a synthesized > constructor added by the compiler. I think generally annotations should > not be associated with synthesized or synthetic parameters. > > -Joe > > On 1/11/2013 9:03 AM, Eric McCorkle wrote: >> Update should be visible now. >> >> On 01/11/13 11:54, Vitaly Davidovich wrote: >>> Yes that's exactly what I'm looking for as well. >>> >>> Sent from my phone >>> >>> On Jan 11, 2013 11:25 AM, "Peter Levart" <peter.lev...@gmail.com >>> <mailto:peter.lev...@gmail.com>> wrote: >>> >>> On 01/11/2013 04:54 PM, Eric McCorkle wrote: >>> >>> The webrev has been updated again. >>> >>> The multiple writes to parameters have been removed, and the >>> tests have >>> been expanded to look at inner classes, and to test modifiers. >>> >>> Please look over it again. >>> >>> >>> Hello Eric, >>> >>> You still have 2 reads of volatile even in fast path. I would do it >>> this way: >>> >>> >>> private Parameter[] privateGetParameters() { >>> Parameter[] tmp = parameters; // one and only read >>> if (tmp != null) >>> return tmp; >>> >>> // Otherwise, go to the JVM to get them >>> tmp = getParameters0(); >>> >>> // If we get back nothing, then synthesize parameters >>> if (tmp == null) { >>> final int num = getParameterCount(); >>> tmp = new Parameter[num]; >>> for (int i = 0; i < num; i++) >>> // TODO: is there a way to synthetically derive the >>> // modifiers? Probably not in the general case, since >>> // we'd have no way of knowing about them, but there >>> // may be specific cases. >>> tmp[i] = new Parameter("arg" + i, 0, this, i); >>> // This avoids possible races from seeing a >>> // half-initialized parameters cache. >>> } >>> >>> parameters = tmp; >>> >>> return tmp; >>> } >>> >>> >>> Regards, Peter >>> >>> >>> Test-wise, I've got a clean run on JPRT (there were some >>> failures in >>> lambda stuff, but I've been seeing that for some time now). >>> >>> On 01/10/13 21:47, Eric McCorkle wrote: >>> >>> On 01/10/13 19:50, Vitaly Davidovich wrote: >>> >>> Hi Eric, >>> >>> Parameter.equals() doesn't need null check - instanceof >>> covers that already. >>> >>> Removed. >>> >>> Maybe this has been mentioned already, but personally >>> I'm not a fan of >>> null checks such as "if (null == x)" - I prefer the >>> null >>> on the right >>> hand side, but that's just stylistic. >>> >>> Changed. >>> >>> Perhaps I'm looking at a stale webrev but >>> Executable.__privateGetParameters() reads and writes >>> from/to the volatile >>> field more than once. I think Peter already mentioned >>> that it should >>> use one read into a local and one write to publish the >>> final version to >>> the field (it can return the temp as well). >>> >>> You weren't. From a pure correctness standpoint, there is >>> nothing wrong >>> with what is there. getParameters0 is a constant >>> function, and >>> parameters is writable only if null. Hence, we only every >>> see one >>> nontrivial write to it. >>> >>> But you are right, it should probably be reduced to a >>> single >>> write, for >>> performance reasons (to avoid unnecessary memory barriers). >>> Therefore, >>> I changed it. >>> >>> However, I won't be able to refresh the webrev until >>> tomorrow. >>> >>> Thanks >>> >>> Sent from my phone >>> >>> On Jan 10, 2013 6:05 PM, "Eric McCorkle" >>> <eric.mccor...@oracle.com >>> <mailto:eric.mccor...@oracle.com> >>> <mailto:eric.mccorkle@oracle.__com >>> <mailto:eric.mccor...@oracle.com>>> wrote: >>> >>> The webrev has been refreshed with the solution I >>> describe below >>> implemented. Please make additional comments. >>> >>> On 01/10/13 17:29, Eric McCorkle wrote: >>> > Good catch there. I made the field volatile, >>> and >>> I also did the same >>> > with the cache fields in Parameter. >>> > >>> > It is possible with what exists that you could >>> wind up with multiple >>> > copies of identical parameter objects in >>> existence. It goes something >>> > like this >>> > >>> > thread A sees Executable.parameters is null, >>> goes >>> into the VM to >>> get them >>> > thread B sees Executable.parameters is null, >>> goes >>> into the VM to >>> get them >>> > thread A stores to Executable.parameters >>> > thread B stores to Executable.parameters >>> > >>> > Since Parameters is immutable (except for its >>> caches, which will >>> always >>> > end up containing the same things), this >>> *should* >>> have no visible >>> > effects, unless someone does == instead of >>> .equals. >>> > >>> > This can be avoided by doing a CAS, which is >>> more >>> expensive >>> execution-wise. >>> > >>> > My vote is to *not* do a CAS, and accept that >>> (in >>> extremely rare >>> cases, >>> > even as far as concurrency-related anomalies >>> go), >>> you may end up with >>> > duplicates, and document that very well. >>> > >>> > Thoughts? >>> > >>> > On 01/10/13 16:10, Peter Levart wrote: >>> >> Hello Eric, >>> >> >>> >> I have another one. Although not very likely, >>> the reference to >>> the same >>> >> Method/Constructor can be shared among multiple >>> threads. The >>> publication >>> >> of a parameters array should therefore be >>> performed via a >>> volatile write >>> >> / volatile read, otherwise it can happen that >>> some thread sees >>> >> half-initialized array content. The >>> 'parameters' >>> field in Executable >>> >> should be declared as volatile and there should >>> be a single read >>> from it >>> >> and a single write to it in the >>> privateGetParameters() method >>> (you need >>> >> a local variable to hold intermediate >>> states)... >>> >> >>> >> Regards, Peter >>> >> >>> >> On 01/10/2013 09:42 PM, Eric McCorkle wrote: >>> >>> Thanks to all for initial reviews; however, it >>> appears that the >>> version >>> >>> you saw was somewhat stale. I've applied your >>> comments (and some >>> >>> changes that I'd made since the version that >>> was posted). >>> >>> >>> >>> Please take a second look. >>> >>> >>> >>> Thanks, >>> >>> Eric >>> >>> >>> >>> >>> >>> On 01/10/13 04:19, Peter Levart wrote: >>> >>>> Hello Eric, >>> >>>> >>> >>>> You must have missed my comment from the >>> previous webrev: >>> >>>> >>> >>>> 292 private Parameter[] >>> privateGetParameters() { >>> >>>> 293 if (null != parameters) >>> >>>> 294 return parameters.get(); >>> >>>> >>> >>>> If/when the 'parameters' SoftReference is >>> cleared, the method >>> will be >>> >>>> returning null forever after... >>> >>>> >>> >>>> You should also retrieve the referent and >>> check for it's >>> presence before >>> >>>> returning it: >>> >>>> >>> >>>> Parameter[] res; >>> >>>> if (parameters != null && (res = >>> parameters.get()) != null) >>> >>>> return res; >>> >>>> ... >>> >>>> ... >>> >>>> >>> >>>> Regards, Peter >>> >>>> >>> >>>> On 01/09/2013 10:55 PM, Eric McCorkle wrote: >>> >>>>> Hello, >>> >>>>> >>> >>>>> Please review the core reflection API >>> implementation of parameter >>> >>>>> reflection. This is the final component of >>> method parameter >>> reflection. >>> >>>>> This was posted for review before, then >>> delayed until the >>> check-in for >>> >>>>> JDK-8004728 (hotspot support for parameter >>> reflection), which >>> occurred >>> >>>>> yesterday. >>> >>>>> >>> >>>>> Note: The check-in of JDK-8004728 was into >>> hsx/hotspot-rt, *not* >>> >>>>> jdk8/tl; therefore, it may be a while before >>> the changeset >>> makes its way >>> >>>>> into jdk8/tl. >>> >>>>> >>> >>>>> Also note: since the check-in of JDK-8004727 >>> (javac support for >>> >>>>> parameter reflection), there has been a >>> failure in the tests for >>> >>>>> Pack200. This is being addressed in a fix >>> contributed by >>> Kumar, which I >>> >>>>> believe has also been posted for review. >>> >>>>> >>> >>>>> The open webrev is here: >>> >>>>> >>> http://cr.openjdk.java.net/~__coleenp/JDK-8004729 >>> <http://cr.openjdk.java.net/~coleenp/JDK-8004729> >>> >>>>> >>> >>>>> The feature request is here: >>> >>>>> >>> http://bugs.sun.com/view_bug.__do?bug_id=8004729 >>> <http://bugs.sun.com/view_bug.do?bug_id=8004729> >>> >>>>> >>> >>>>> The latest version of the spec can be >>> found here: >>> >>>>> >>> http://cr.openjdk.java.net/~__abuckley/8misc.pdf >>> <http://cr.openjdk.java.net/~abuckley/8misc.pdf> >>> >>>>> >>> >>>>> >>> >>>>> Thanks, >>> >>>>> Eric >>> >> >>> >>> >
