Are there any additional comments? I'd like to get this pushed today.
On 01/17/13 16:54, Eric McCorkle wrote: > 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 >>>> >> >>>> >>>> >>
